Summary:
Ref T4420. This was a performance hack introduced long ago to make typeaheads for users a little cheaper. The idea was that you could load some of an object's columns and skip other ones.
We now always load users on demand, so the cost of loading the whole objects is very small. No other use cases ever arose for this, and it seems unlikely that they will in the future. Remove it all.
Test Plan:
- Grepped for `CONFIG_PARTIAL_OBJECTS`.
- Grepped for `dirtyFields`.
- Grepped for `missingFields`.
- Grepped for `resetDirtyFields`.
- Grepped for `loadColumns`.
- Grepped for `loadColumnsWhere`.
- Grepped for `loadRawDataWhere`.
- Loaded and saved some lisk objects.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4420
Differential Revision: https://secure.phabricator.com/D9895
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: Ref T5179. Ref T4045. Ref T832. We can now write non-utf8 hunks into the database, so try to do more reasonable things with them in the UI.
Test Plan: (See screenshots...)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T832, T4045, T5179
Differential Revision: https://secure.phabricator.com/D9294
Summary: Ran `arc lint --apply-patches --everything` over rP, mainly to change double quotes to single quotes where appropriate. These changes also validate that the `ArcanistXHPASTLinter::LINT_DOUBLE_QUOTE` rule is working as expected.
Test Plan: Eyeballed it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9431
Summary:
Ref T2222.
- Removes `DifferentialTasksAttacher`, which has had no callsites for a very long time.
- Moves `differential.getrevisioncomments` off `DifferentialCommentQuery`.
- Moves Releeph churn field off `DifferentialCommentQuery`.
- Removes dead code in `DifferentialRevisionViewController`.
- Removes `DifferentialException` (no references).
- Removes `DifferentialRevision->loadComments()` (no callsites).
- Removes `DifferentialRevision->loadReviewedBy()` (all callsites updated).
- Removes `DifferentialCommentQuery` (all callsites updated).
Test Plan: Mostly a lot of `grep`.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D8476
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
Summary: Ref T4570. Add trivial assertions to tests which fail-by-exploding so we can fail tests with no assertions.
Test Plan: Ran `arc unit --everything` with Arcanist patched to fail with no assertions.
Reviewers: leebyron, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4570
Differential Revision: https://secure.phabricator.com/D8436
Summary:
Ref T1191. I believe we only have three meaningful binary fields across all applications:
- The general cache may contain gzipped content.
- The file storage blob may contain arbitrary binary content.
- The Passphrase secret can store arbitrary binary data (although it currently never does).
This adds Lisk config for binary fields, and uses `%B` where necessary.
Test Plan:
- Added and executed unit tests.
- Forced file uploads to use MySQL, uploaded binaries.
- Disabled the CONFIG_BINARY on the file storage blob and tried again, got an appropraite failure.
- Tried to register with an account containing a G-Clef, and was stopped before the insert.
Reviewers: btrahan, arice
Reviewed By: arice
CC: arice, chad, aran
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D8316
Summary: Ref T4375. We never join this table, so this is a pretty straight find/replace.
Test Plan: Browsed around Calendar, verified that nothing seemed broken. Saw my red dot in other apps.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4375
Differential Revision: https://secure.phabricator.com/D8145
Summary:
Ref T4327. I want to make change parsing testable; one thing which is blocking this is that the Git discovery process is still part of `PullLocal` daemon instead of being part of `DiscoveryEngine`. The unit test stuff which I want to use for change parsing relies on `DiscoveryEngine` to discover repositories during unit tests.
The major reason git discovery isn't part of `DiscoveryEngine` is that it relies on the messy "autoclose" logic, which we never implemented for Mercurial. Generally, I don't like how autoclose was implemented: it's complicated and gross and too hard to figure out and extend.
Instead, I want to do something more similar to what we do for pushes, which is cleaner overall. Basically this means remembering the old branch heads from the last time we parsed a repository, and figuring out what's new by comparing the old and new branch heads. This should give us several advantages:
- It should be simpler to understand than the autoclose stuff, which is pretty mind-numbing, at least for me.
- It will let us satisfy branch and tag queries cheaply (from the database) instead of having to go to the repository. We could also satisfy some ref-resolve queries from the database.
- It should be easier to extend to Mercurial.
This implements the basics -- pretty much a table to store the cursors, which we update only for Git for now.
Test Plan:
- Ran migration.
- Ran `bin/repository discover X --trace --verbose` on various repositories with branches and tags, before and after modifying pushes.
- Pushed commits to a git repo.
- Looked at database tables.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T4327
Differential Revision: https://secure.phabricator.com/D7982
Summary:
If you do something like this:
// Missing $user->getPHID()!
$object->setUserPHID($user)->save();
...you get a very unhelpful exception:
Expected a scalar or null for %s conversion. Query: %s
This doesn't give you any hints about what's wrong. Instead, provide a more useful exception:
Unable to insert or update object of class DifferentialRevision, field 'title' has a nonscalar value.
Test Plan: {F87614}
Reviewers: hach-que, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D7725
Summary:
Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.
Some policy objects don't have real PHIDs:
PhabricatorTokenGiven
PhabricatorSavedQuery
PhabricatorNamedQuery
PhrequentUserTime
PhabricatorFlag
PhabricatorDaemonLog
PhabricatorConduitMethodCallLog
ConduitAPIMethod
PhabricatorChatLogEvent
PhabricatorChatLogChannel
Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.
Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.
Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7306
Summary:
Fixes T3821. Maybe. The existing code seemed to have a bug and actually return the //commit phid//. Judging by the function name this is not intended.
Also, sorry to step on toes here -- I thought no one was assigned and was curious about loadRelativeEdges and here we are...
Test Plan: lots of logic here as I have no idea how to use Releeph.
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3821
Differential Revision: https://secure.phabricator.com/D6967
Summary: I'm going to just delete all this code at some point, but fixing it now means piles of single gets, so unbreak it first. I'll file something.
Test Plan: Releeph is less fataley.
Reviewers: andrewjcg, btrahan
Reviewed By: andrewjcg
CC: aran
Differential Revision: https://secure.phabricator.com/D6945
Summary:
Ref T3599
Go through everything, grep a bit, replace some bits.
Test Plan: Navigate around a bit
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, aran
Maniphest Tasks: T3599
Differential Revision: https://secure.phabricator.com/D6871
Summary:
Ref T2715. Ref T3551. Ref T603. This does a few things, but they're all sort of small:
- We commonly use a `getX()` / `attachX()` pattern, but have very similar code in the `getX()` method every time. Provide a convenience method to make this pattern easier to write.
- We use `willFilterPage()` in many queries, but it currently is called with zero or more results. This means we have a lot of "if no results, return nothing" boilerplate. Make it call only for one or more results.
- Implement `PhabricatorPolicyInterface` on `ReleephBranch`. A branch has the same policy as its project.
- Implement `ReleephBranchQuery`.
- Move the branch PHID type to application PHID infrastructure.
Test Plan: Browsed Releeph. Used `phid.query` to query branch PHIDs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T2715, T3551
Differential Revision: https://secure.phabricator.com/D6512
Summary: Ref T2222. I need this to migrate the Differential comment tables, because they are large. We have the similar `LiskMigrationIterator` already, but it won't work here because I intend to destroy the original objects after migrating them.
Test Plan:
Wrote a script to iterate over the `differential_comment` table, got reasonable output:
{P869}
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T2222
Differential Revision: https://secure.phabricator.com/D6264
Test Plan: Searched for `IDS_PHID` and loaded homepage.
Reviewers: epriestley, edward
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D5619
Summary:
Remarkup rule callbacks now get SafeHTML matches instead of string matches. If they call:
$some_lisk_dao->load($matches[1]);
..as is the case with the `{F123}` rule, we reject the SafeHTML as an invalid ID and return null.
Allow load() to string convert any object (which will either succeed or fatal in an obviously-broken way).
(Long ago we threw instead of returning null here, but it meant we had to do a lot of redundant checks.)
Test Plan: `{F123}` shows an image again. `{C1}` embeds a countdown.
Reviewers: vrana, chad
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4961
Summary:
T2345
getConfig throws an Exception when the key does not exist.
Also removes dead code that throws an Exception.
Test Plan:
Reloaded the Phabricator home page. In the process, found
2 Exceptions thrown due to nonexistent keys. After addressing these problems,
the home page loads without Exceptions.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4541
Summary:
- Ports MySQL settings to PHP.
- Removes "mysql.retries" -- this existed only because Magic Numbers Are Bad, but there is no concievable reason it should ever be set to anything other than 3.
- Introduced "Hidden" config, which isn't visible from the web (for SaaS, we'll just mark anything with secret keys as "hidden").
- Introduced "Masked" config, which will be masked in darkconsole once that gets updated.
- "Hidden" implies "Masked" and "Locked".
- Moved "storage.default-namespace" here -- it probably makes more sense than core; this was my bad in T2255.
- Put cancel button back for hidden/locked config.
- Introduce 'class' config type.
Test Plan: Viewed MySQL options. None are editable.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4326
Summary:
See discussion in D4204. Facebook currently has a 314MB remarkup cache with a 55MB index, which is slow to access. Under the theory that this is an index size/quality problem (the current index is on a potentially-384-byte field, with many keys sharing prefixes), provide a more general index with fancy new features:
- It implements PhutilKeyValueCache, so it can be a component in cache stacks and supports TTL.
- It has a 12-byte hash-based key.
- It automatically compresses large blocks of data (most of what we store is highly-compressible HTML).
Test Plan:
- Basics:
- Loaded /paste/, saw caches generate and save.
- Reloaded /paste/, saw the page hit cache.
- GC:
- Ran GC daemon, saw nothing.
- Set maximum lifetime to 1 second, ran GC daemon, saw it collect the entire cache.
- Deflate:
- Selected row formats from the database, saw a mixture of 'raw' and 'deflate' storage.
- Used profiler to verify that 'deflate' is fast (12 calls @ 220us on my paste list).
- Ran unit tests
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4259
Summary: See D3912 for discussion. InnoDB may reuse autoincrement IDs after restart; provide a way to avoid it.
Test Plan: Unit tests. Scheduled and executed tasks through `drydock lease --type host` and `phd debug taskmaster`.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3914
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
See comment.
This can reveal some pretty bad bugs but HPHP handles this correctly so we already know about them.
Test Plan:
Added `phlog()` to `__call()` and observed what is defined for each method (under PHP). Also:
class C {
function __call($name, $args) {
static $class;
if (!$class) {
$class = get_class($this);
}
return $class;
}
}
class D extends C {
}
class E extends C {
}
$d = new D;
$e = new E;
var_dump($d->x());
var_dump($e->x()); // Prints D under PHP!
See also D3754.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1261
Differential Revision: https://secure.phabricator.com/D3753
Summary: For immutable objects, just use the ID as a cursor.
Test Plan:
- Analyzed commits from an empty cursor.
- Checked that cursor was good.
- Pulled some more commits.
- Analyzed commits again, verified it only hit the new ones.
- Verified the graph of "Count of CMIT" looked reasonable.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1866
Differential Revision: https://secure.phabricator.com/D3656
Summary: We never use this and almost certainly never will. It's been in Lisk for ~7 years but is a solution in search of a problem. It causes a conflict with any DAO that has a `version` column.
Test Plan: Browsed around, performed inserts and updates. Edited a Phriction document.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran, leslie.chong
Differential Revision: https://secure.phabricator.com/D3625
Summary:
Fixes a TODO, and silences a warning introduced by D3601.
There are several cases where we load data like:
SELECT *, ... AS extraData FROM ...
...and then pass it to `loadAllFromArray()`. Currently, this causes us to set an `extraData` property on the object.
This idiom seems fairly useful and non-dangerous, so I made `loadFromArray()` just drop extra keys.
Since we hit this loop a potentially huge number of times (10,000+ for full Maniphest pages) I did some microoptimization. Lisk is hot enough that it's one of the few places where it's worthwhile (see D1291).
Test Plan: Loaded homepage, no longer got warnings about `viewerIsMember` from Project queries. Browsed ~10 apps, didn't see any issues.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3606
Summary:
I make this error quite often: I forget to declare a property I am writing to or I make a typo in it.
PHP implicitly creates a public property which I don't like.
I would much rather see a linter warning me against this than this runtime check but writing it is very difficult:
- We need to explore all parents of the class we are checking.
- It is even possible that children will declare that property but it's OK to treat this as error anyway.
- We can extend also builtin or external classes.
- It's somewhat doable for `$this` but even more complex for any `$obj` because we don't know the class of it.
This should catch significant part of these errors and I'm fine with that.
I don't plan escalating to exception because this error is not fatal and should not stop the application from working.
Test Plan: Loaded homepage, checked log.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3601
Summary:
Calling `->setPHID()` or other common Lisk setters creates an implicit public property `$phid`.
I don't like implicit properties and I see them as errors.
Its public visibility also makes me nervous and is vulnerable to bypassing any setters we may create.
Test Plan: Loaded homepage, checked log.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3600
Summary:
We have two troubles with this script:
# Our revisions and commits don't fit in the memory. (Our tasks do :-).)
# Reindexing revisions is slow.
Test Plan: Ran it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3483
Summary:
More and more relations are going under edges and I can't work with them from Relatives framework.
This doesn't have the nice transitive property of normal relatives (loading relative objects from relatives loads all of them at once) but I can add it when I need it.
I plan to use it in D3085 (after converting relationships to edges).
Test Plan:
$task = id(new ManiphestTask())
->loadOneWhere('phid = %s', $phid);
print_r($task->loadRelativeEdges(4));
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3344
Summary:
Fact engines loading dependent objects are super slow because they load them one by one.
This diff put each page in a Lisk set allowing engines to use `loadRelatives()`.
It also introduces `clearSet()` method which is somewhat neccessary in PHP < 5.3 or with disabled cyclic [[ http://php.net/gc | GC ]].
Test Plan:
$iterator = new PhabricatorFactUpdateIterator(new DifferentialRevision());
foreach ($iterator as $revision) {
$diffs = $revision->loadRelatives(new DifferentialDiff(), 'revisionID');
echo memory_get_usage() . "\n";
}
Experienced not-steadily-increasing memory usage and much faster loading.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3247
Summary: PhutilBufferedIterator now implements all the nonspecific logic here.
Test Plan:
Created a test script like this:
$iterator = new LiskMigrationIterator(new DifferentialRevision());
$iterator->setPageSize(3);
foreach ($iterator as $key => $rev) {
echo "{$key}: ".$rev->getID()."\n";
}
Ran it and verified sensible iteration results.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1562
Differential Revision: https://secure.phabricator.com/D3076
Summary: We pull "retries" and a doc link from PhabricatorEnv directly. Break these dependencies so the classes can move to libphutil.
Test Plan: Browsed site, triggered a schema exception and verified I still got the useful footer text.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1283
Differential Revision: https://secure.phabricator.com/D3053
Summary: See D3006, D3007. Make it easier to do migrations like that without holding all results in memory.
Test Plan:
Ran this code with an artificially small page size (2):
foreach (new LiskMigrationIterator(new DifferentialRevision()) as $rev) {
echo "Revision ".$rev->getID()."\n";
}
Verified each revision as loaded and processed.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D3008
Summary:
Lisk currently behaves in two different ways if you call it like `load("cow")` (throws) versus `load(99999999)` (returns null), where neither ID exists.
This was intended to catch programming errors as distinct from missing data, but in practice the former is very rare and you have to handle the latter in most cases anyway. The case where you pass "0" is particularly confusing. See D2971 for an example.
On the balance, I think this ends up being far more confusing than helpful. Instead, just return NULL if we're sure there's no such object.
Test Plan: Reasoned about program behavior.
Reviewers: alanh, btrahan, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2977
Summary:
In unit tests which use fixtures, we open transactions on every connection we establish. However, since we don't track connections that are established with "$force_new" (currently, only GlobalLock connections) we never close these transactions normally.
Instead of not tracking these connections, track them using unique keys so we'll never get a cache hit on them.
Test Plan: Built unit tests on top of this, had them stop dying from unclosed transactions.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1162
Differential Revision: https://secure.phabricator.com/D2938
