Summary: Fixes T11201.
Test Plan:
Created bogus edges like this:
```
INSERT INTO edge (src, type, dst, dateCreated, seq) values ('PHID-TASK-vnddativbialb5p6ymis', 999999, 'quack', UNIX_TIMESTAMP(), 1);
```
Then ran `bin/remove destroy` on the relevant object.
Before the patch, destruction halted after hittin the bad edge.
After the patch, a warning is emitted but destruction continues.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11201
Differential Revision: https://secure.phabricator.com/D16171
Summary:
Ref T11179. This splits "Edit Blocking Tasks" into two options now that we have more room ("Edit Parent Tasks", "Edit Subtasks").
This also renames "Blocking" tasks to "Subtasks", and "Blocked" tasks to "Parent" tasks. My goals here are:
- Make the relationship direction more clear: it's more clear which way is up with "parent" and "subtask" at a glance than with "blocking" and "blocked" or "dependent" and "dependency".
- Align language with "Create Subtask".
- To some small degree, use more flexible/general-purpose language, although I haven't seen any real confusion here.
Fixes T6815. I think I narrowed this down to two issues:
- Just throwing a bare exeception (we now return a dialog explicitly).
- Not killing open transactions when the cyclec check fails (we now kill them).
Test Plan:
- Edited parent tasks.
- Edited subtasks.
- Tried to introduce graph cycles, got a nice error dialog.
{F1697087}
{F1697088}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6815, T11179
Differential Revision: https://secure.phabricator.com/D16166
Summary: Ref T4280. At some point (probably D15732) we started getting anchor parsing wrong. Just pop the anchor off before doing all the logic, then put it back on at the end.
Test Plan:
Tested various forms like:
```
[[ x ]]
[[ x | z ]]
[[ x#y | z ]]
[[ ./x#y | z ]]
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4280
Differential Revision: https://secure.phabricator.com/D16083
Summary:
Ref T4103. Ref T10078. Currently, when a user misses a cache we just build it for them.
This is the behavior we want for the the viewer (so we don't have to build every cache up front if we don't actually need them), but not the right behavior for other users (since it allows performance problems to go undetected).
Make inline cache generation strict by default, then make sure all the things that rely on cache data request the correct data (well, all of the things identified by unit tests, at least: there might be some more stuff I haven't hit yet).
This fixes test failures in D16040, and backports a piece of that change.
Test Plan: Identified and then fixed failures with `arc unit --everything`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103, T10078
Differential Revision: https://secure.phabricator.com/D16042
Summary:
Ref T4103. These are currently stored on the user, for historic/performance reasons.
Since I want administrators to be able to set defaults for translations and timezones at a minimum and there's no longer a meaningful performance penalty for moving them off the user record, turn them into real preferences and then nuke the columns.
Test Plan:
- Set settings to unusual values.
- Ran migrations.
- Verified my unusual settings survived.
- Created a new user.
- Edited all settings with old and new UIs.
- Reconciled client/server timezone disagreement.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4103
Differential Revision: https://secure.phabricator.com/D16005
Summary:
Ref T6299. This makes more of the links point to the right places.
Not covered yet:
- Projects and subscribers don't point to the right place (this is a little tricky to fix, I think).
- `[[ #anchor ]]`s won't do the right thing in, uh, email, I guess, since `uri.here` is not set. This is also a little tricky.
Possibly we should just remove subscribers (although also kind of tricky).
Test Plan: On a custom-domain blog, observed that fewer things were broken.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6299
Differential Revision: https://secure.phabricator.com/D16007
Summary:
Fixes T11042. Currently, all `bin/storage` workflows stop if `cluster.read-only` is set:
```
$ ./bin/storage adjust
Usage Exception: Phabricator is currently in read-only mode. Use --force to override this mode.
```
However, some of them (`status`, `dump`, `databases`, etc) are read-only anyway and safe to run. Don't prompt in these cases.
Test Plan:
- Set `cluster.read-only` to `true`.
- Ran `bin/storage dump`, `bin/storage status`, etc. No longer received messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11042
Differential Revision: https://secure.phabricator.com/D15987
Summary:
Ref T5267. Fixes T9643. Currently, we can not parse/extract a small fraction of strings:
- PHP files which contain obscure syntax that XHPAST can not parse.
- HEREDOCs do not have a useable `evalStatic()` behavior right now.
Emit these as warnings, but continue and generate all usable/extractable translations.
Test Plan:
```
epriestley@orbital ~/dev/phabricator $ ./bin/i18n extract . >/dev/null
Found 4,548 files...
Done.
WARNING: Failed to extract strings from file "/Users/epriestley/dev/core/lib/phabricator/externals/stripe-php/lib/Stripe/ApiRequestor.php": XHPAST Parse Error: syntax error, unexpected T_STRING on line 357
WARNING: Failed to evaluate pht() call on line 141 in "/Users/epriestley/dev/core/lib/phabricator/scripts/repository/commit_hook.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 24 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorClusterConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 23 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 49 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 83 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 91 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 97 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 103 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 113 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 119 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 128 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 137 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 149 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 159 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 166 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 179 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 192 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorMetaMTAConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 24 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/config/option/PhabricatorNotificationConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 26 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/harbormaster/step/HarbormasterCircleCIBuildStepImplementation.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 154 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 282 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/maniphest/config/PhabricatorManiphestConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 84 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/maniphest/editor/ManiphestEditEngine.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 345 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/metamta/storage/PhabricatorMetaMTAReceivedMail.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 59 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/owners/editor/PhabricatorOwnersPackageEditEngine.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 26 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/project/config/PhabricatorProjectConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 53 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/project/config/PhabricatorProjectConfigOptions.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 98 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 165 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 252 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 299 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 360 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 439 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 529 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/search/engine/PhabricatorSearchEngineAPIMethod.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 54 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/controller/PhabricatorEditEngineConfigurationLockController.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 56 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 126 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 185 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 238 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 288 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/transactions/view/PhabricatorApplicationEditHTTPParameterHelpView.php": Unexpected node during static evaluation, of type: n_HEREDOC
WARNING: Failed to evaluate pht() call on line 17 in "/Users/epriestley/dev/core/lib/phabricator/src/applications/uiexample/examples/PhabricatorRemarkupUIExample.php": Unexpected node during static evaluation, of type: n_HEREDOC
epriestley@orbital ~/dev/phabricator $
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5267, T9643
Differential Revision: https://secure.phabricator.com/D15983
Summary: Ref T5267. Put "Deutsch" in the list instead of "German", so you can find your language without knowing the English word for it.
Test Plan: {F1661598}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5267
Differential Revision: https://secure.phabricator.com/D15980
Summary:
Fixes T10381. When we converted to `PHUIRemarkupView`, some instructional text got linebreaks added when it shouldn't have them (the source is written in PHP and wrapped at 80 characters, but the output should flow naturally).
Fix this so we don't preserve linebreaks.
This also makes `PHUIRemarkupView` a little more powerful and inches us toward fixing Phame/CORGI remarkup issues, getting rid of `PhabricatorMarkupInterface` / `PhabricatorMarkupOneOff`, and dropping all the application hard-coding in `PhabricatorMarkupEngine`.
Test Plan:
- Grepped for all callsites, looking for callsites which accept remarkup written in `<<<HEREDOC` format.
- Viewed form instructions, Conduit API methods, HTTP parameter edit instructions.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10381
Differential Revision: https://secure.phabricator.com/D15963
Summary: Fixes T10941. This avoids a confusing dead end when configuring Subversion hosting, where `svnserve` will fail to execute hooks if the CWD isn't readable by the vcs-user.
Test Plan:
- Updated and committed in a hosted SVN repository.
- Ran some git operations, too.
- @dpotter confirmed this locally in T10941.
Reviewers: chad
Reviewed By: chad
Subscribers: dpotter
Maniphest Tasks: T10941
Differential Revision: https://secure.phabricator.com/D15879
Summary: Ref T9790. This passes the map down so we can generate highlighted mail.
Test Plan:
Generated this relatively respectable-looking HTML mail:
{F1258558}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790
Differential Revision: https://secure.phabricator.com/D15848
Summary:
Ref T9790. This prepares the syntax color rules to be reused in mail.
This goes about halfway toward T5701 by sort-of supporting different styles but not really.
Test Plan:
- Ran `bin/celerity syntax` to regenerate syntax map.
- Viewed some highlighted code, didn't see any differences.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9790
Differential Revision: https://secure.phabricator.com/D15846
Summary:
Ref T10748. Ref T10366. Allows users to set credential for new URIs.
- Ref T7221. Our handling of the "git://" protocol is currently incorrect. This protocol is not authenticated, but is considered an SSH protocol. In the new UI, it is considered an anonymous/unauthenticated protocol instead.
- Ref T10241. This fixes the `PassphraseCredentialControl` so it doesn't silently edit the value if the current value is not visible to you and/or not valid.
Test Plan:
Performed a whole lot of credential edits, removals, and adjustments. I'll give this additional vetting before cutting over to it.
{F1253207}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7221, T10241, T10366, T10748
Differential Revision: https://secure.phabricator.com/D15829
Summary:
Ref T10849. This enforces a global 30-second per-query time limit for anything not coming from the CLI.
If we run into another issue with MySQL hanging in the future, this should prevent it from being nearly as bad as it was.
Test Plan:
- Set value to 0, verified the UI threw an exception immediately.
- Set value back to 30, browsed around a bunch of pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10849
Differential Revision: https://secure.phabricator.com/D15799
Summary:
Ref T10860. This allows us to recover if the connection to the database is lost during a push.
If we lose the connection to the master database during a push, we would previously freeze the repository. This is very safe, but not very operator-friendly since you have to go manually unfreeze it.
We don't need to be quite this aggressive about freezing things. The repository state is still consistent after we've "upgraded" the lock by setting `isWriting = 1`, so we're actually fine even if we lost the global lock.
Instead of just freezing the repository immediately, sit there in a loop waiting for the master to come back up for a few minutes. If it recovers, we can release the lock and everything will be OK again.
Basically, the changes are:
- If we can't release the lock at first, sit in a loop trying really hard to release it for a while.
- Add a unique lock identifier so we can be certain we're only releasing //our// lock no matter what else is going on.
- Do the version reads on the same connection holding the lock, so we can be sure we haven't lost the lock before we do that read.
Test Plan:
- Added a `sleep(10)` after accepting the write but before releasing the lock so I could run `mysqld stop` and force this issue to occur.
- Pushed like this:
```
$ echo D >> record && git commit -am D && git push
[master 707ecc3] D
1 file changed, 1 insertion(+)
# Push received by "local001.phacility.net", forwarding to cluster host.
# Waiting up to 120 second(s) for a cluster write lock...
# Acquired write lock immediately.
# Waiting up to 120 second(s) for a cluster read lock on "local001.phacility.net"...
# Acquired read lock immediately.
# Device "local001.phacility.net" is already a cluster leader and does not need to be synchronized.
# Ready to receive on cluster host "local001.phacility.net".
Counting objects: 3, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (2/2), done.
Writing objects: 100% (3/3), 254 bytes | 0 bytes/s, done.
Total 3 (delta 1), reused 0 (delta 0)
BEGIN SLEEP
```
- Here, I stopped `mysqld` from the CLI in another terminal window.
```
END SLEEP
# CRITICAL. Failed to release cluster write lock!
# The connection to the master database was lost while receiving the write.
# This process will spend 300 more second(s) attempting to recover, then give up.
```
- Here, I started `mysqld` again.
```
# RECOVERED. Link to master database was restored.
# Released cluster write lock.
To ssh://local@localvault.phacility.com/diffusion/26/locktopia.git
2cbf87c..707ecc3 master -> master
```
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10860
Differential Revision: https://secure.phabricator.com/D15792
Summary:
Trigger daemon is trying to find the next event to invoke before sleeping, but the query includes already-elapsed triggers.
It then tries to sleep for 0 seconds.
Test Plan:
On a new instance, schedule a single trigger of type `PhabricatorOneTimeTriggerClock` to a very near time.
Use top to see trigger daemon not going to 100% CPU once the event has elapsed.
Reviewers: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15750
Summary:
Fixes T10830.
- The return code from `storage adjust` did not propagate correct.
- There was one column issue which I missed the first time around because I had a bunch of unrelated stuff locally.
Test Plan:
- Ran `bin/storage upgrade -f` with failures, used `echo $?` to make sure it exited nonzero.
- Got fully clean `bin/storage adjust` by dropping all my extra local tables.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10830
Differential Revision: https://secure.phabricator.com/D15746
Summary:
Fixes T10830. Ref T10366. I wasn't writing to this table yet so I didn't build it, but the fact that `bin/storage adjust` would complain slipped my mind.
- Add the table.
- Make the tests run `adjust`. This is a little slow (a few extra seconds) but we could eventually move some steps like this to run server-side only.
Test Plan: Ran `bin/storage upgrade -f`, got a clean `adjust`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10366, T10830
Differential Revision: https://secure.phabricator.com/D15744
Summary:
Fixes T7475. If you do something like:
$ umask 123
$ ./bin/phd start
...the daemons might inherit the weird umask, do a `git fetch` with the weird umask, and end up creating files with weird permissions in repositories.
Instead, just normalize the umask to 022 in all cases. This is overwhelmingly the most common setting, and the one we assume things are configured with.
(When we want to force permissions to a certain setting, we do so explicitly.)
Test Plan:
- Added `var_dump(umask())` to observe umask.
- Ran `bin/phd`, saw proper umask (`18`, which is decimal of `022` octal).
- Set `umask 123`, then ran `bin/phd`, saw it correct properly again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7475
Differential Revision: https://secure.phabricator.com/D15721
Summary: These flags do slightly different things, I actually want --master-data here. My test databases are setup half-weird and work with either statement, which is why I missed this.
Test Plan: Ran a dump against master, got the right CHANGE MASTER statement with no warnings.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15716
Summary:
Ref T6915. This allows multiple notification servers to talk to each other:
- Every server has a list of every other server, including itself.
- Every server generates a unique fingerprint at startup, like "XjeHuPKPBKHUmXkB".
- Every time a server gets a message, it marks it with its personal fingerprint, then sends it to every other server.
- Servers do not retransmit messages that they've already seen (already marked with their fingerprint).
- Servers learn other servers' fingerprints after they send them a message, and stop sending them messages they've already seen.
This is pretty crude, and the first message to a cluster will transmit N^2 times, but N is going to be like 3 or 4 in even the most extreme cases for a very long time.
The fingerprinting stops cycles, and stops servers from sending themselves copies of messages.
We don't need to do anything more sophisticated than this because it's fine if some notifications get lost when a server dies. Clients will reconnect after a short period of time and life will continue.
Test Plan:
- Wrote two server configs.
- Started two servers.
- Told Phabricator about all four services.
- Loaded Chrome and Safari.
- Saw them connect to different servers.
- Sent messages in one, got notifications in the other (magic!).
- Saw the fingerprinting stuff work on the console, no infinite retransmission of messages, etc.
(This pretty much just worked when I ran it the first time so I probably missed something?)
{F1218835}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6915
Differential Revision: https://secure.phabricator.com/D15711
Summary:
Fixes T10758.
- Adds a "--host" flag. If you specify this, we read your cluster config. This lets you dump from a replica.
- Adds a "--for-replica" flag to `storage dump`. This makes `mysqldump` include a `CHANGE MASTER ...` statement in the output, which is useful when setting up a replica for the first time.
Test Plan:
- Dumped master and replica cluster databases.
- Dumped non-cluster databases.
- Ran various other commands (help, status, etc).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10758
Differential Revision: https://secure.phabricator.com/D15714
Summary:
Fixes T10697. This finishes bringing the rest of the config up to cluster power levels.
Phabricator is now given an arbitrarily long list of notification servers.
Each Aphlict server is given an arbitrarily long list of ports to run services on.
Users are free to make them meet in the middle by proxying whatever they want to whatever else they want.
This should also accommodate clustering fairly easily in the future.
Also rewrote the status UI and changed a million other things. 🐗
Test Plan:
{F1217864}
{F1217865}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10697
Differential Revision: https://secure.phabricator.com/D15703
Summary:
Ref T4571. Write more of the missing documentation sections and clarify a few things.
Since the "replicating master" check needs a special permission, imposes a performance penalty, is probably very difficult to misconfigure, and likely not a big deal anyway, just drop the idea of trying to automatically detect + prevent it. We still show if it's an issue on the status page, provided we have permission to check.
When you don't have any cluster databases configured, never stop trying to connect to the default master database. We might want to do this eventually as load reduction, but just don't muddy the waters too much for now while things stabilize.
Test Plan:
- Tested functionality in cluster, non-cluster, and degraded-cluster modes.
- Used status console to monitor a health check cycle.
- Read docs.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15679
Summary:
Ref T4571. When a database goes down briefly, we fall back to replicas.
However, this fallback is slow (not good for users) and keeps sending a lot of traffic to the master (might be bad if the root cause is load-related).
Keep track of recent connections and fully degrade into "severed" mode if we see a sequence of failures over a reasonable period of time. In this mode, we send much less traffic to the master (faster for users; less load for the database).
We do send a little bit of traffic still, and if the master recovers we'll recover back into normal mode seeing several connections in a row succeed.
This is similar to what most load balancers do when pulling web servers in and out of pools.
For now, the specific numbers are:
- We do at most one health check every 3 seconds.
- If 5 checks in a row fail or succeed, we sever or un-sever the database (so it takes about 15 seconds to switch modes).
- If the database is currently marked unhealthy, we reduce timeouts and retries when connecting to it.
Test Plan:
- Configured a bad `master`.
- Browsed around for a bit, initially saw "unrechable master" errors.
- After about 15 seconds, saw "major interruption" errors instead.
- Fixed the config for `master`.
- Browsed around for a while longer.
- After about 15 seconds, things recovered.
- Used "Cluster Databases" console to keep an eye on health checks: it now shows how many recent health checks were good:
{F1213397}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15677
Summary:
Ref T4571. If we fail to connect to the master, automatically try to degrade into a temporary read-only mode ("UNREACHABLE") for the remainder of the request, if possible.
If the request was something like "load the homepage", that'll work fine. If it was something like "submit a comment", there's nothing we can do and we just have to fail.
Detecting this condition imposes a performance penalty: every request checks the connection and gives the database a long time to respond, since we don't want to drop writes unless we have to. So the degraded mode works, but it's really slow, and may perpetuate the problem if the root issue is load-related.
This lays the groundwork for improving this case by degrading futher into a "SEVERED" mode which will persist across requests. In the future, if several requests in a short period of time fail, we'll sever the database host and refuse to try to connect to it for a little while, connecting directly to replicas instead (basically, we're "health checking" the master, like a load balancer would health check a web application server). This will give us a better (much faster) degraded mode in a major service disruption, and reduce load on the master if the root cause is load-related, giving it a better chance of recovering on its own.
Test Plan:
- Disabled master in config by changing the host/username, got degraded automatically to UNREACAHBLE mode immediately.
- Faked full SEVERED mode, requests hit replicas and put me in the mode properly.
- Made stuff work, hit some good pages.
- Hit some non-cluster pages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15674
Summary: Ref T4571. If `cluster.databases` is configured but only has replicas, implicitly drop to read-only mode and send writes to a replica.
Test Plan:
- Disabled the `master`, saw Phabricator automatically degrade into read-only mode against replicas.
- (Also tested: explicit read-only mode, non-cluster mode, properly configured cluster mode).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15672
Summary:
Ref T4571. Allows users to click the "read-only mode" notification to get more information about why an install is in read-only mode.
Installs can be in this mode for several reasons (explicit administrative action, no masters defined, no masters reachable), and it's useful to be able to tell the difference.
Test Plan: {F1212930}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15671
Summary: Fixes T6710. After D15669, we support a proper timeout parameter, so we don't need this hack anymore.
Test Plan: See D15669: forced a MySQL connector, set a low timeout, set a bad database, saw fast failures.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6710
Differential Revision: https://secure.phabricator.com/D15670
Summary:
Ref T4571. Ref T10759. Ref T10758. This isn't complete, but gets most of the job done:
- When `cluster.databases` is set up, most things ignore `mysql.host` now.
- You can `bin/storage upgrade` and stuff works.
- You can browse around in the web UI and stuff works.
There's still a lot of weird tricky stuff to navigate, and this has real no advantages over configuring a single server yet (no automatic failover, etc).
Test Plan:
- Configured `cluster.databases` to point at my `t1.micro` hosts in EC2 (master + replica).
- Ran `bin/storage upgrade`, got a new install setup on them properly.
- Survived setup warnings, browsed around.
- Switched back to local config, ran `bin/storage upgrade`, browsed around, went through setup checks.
- Intentionally broke config (bad hosts, no masters) and things seemed to react reasonably well.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571, T10758, T10759
Differential Revision: https://secure.phabricator.com/D15668
Summary: Ref T4571. The configuration option still doesn't do anything, but add a status panel for basic setup monitoring.
Test Plan:
Here's what a good version looks like:
{F1212291}
Also faked most of the errors it can detect and got helpful diagnostic messages like this:
{F1212292}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15667
Summary:
Ref T4571. This adds a new option which allows you to upgrade your one-host configuration to a multi-host configuration by configuring it.
Doing this currently does nothing. I wrote a lot of words about what it is //supposed// to do in the future, though.
Test Plan:
- Tried to configure the option in all the possible bad ways, got errors.
- Read documentation.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15663
Summary:
Ref T4571. There will be a very long path beyond this, but add a basic read-only mode. You can explicitly enable this to put Phabricator in a sort of "maintenance" mode today if you're swapping databases or something.
In the long term, we'll automatically degrade into this mode if the master database is down.
Test Plan:
- Enabled read-only mode.
- Browsed around.
- Didn't immediately see anything that was totally 100% broken.
Most stuff is 80-90% broken right now. For example:
- Stuff like submitting comments doesn't work, and gives you a confusing, unhelpful error.
- None of the UI really knows that it's read-only. EditEngine stuff should all hide itself and say "you can't add new comments while an install is in read-only mode", for example, but currently does not.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T4571
Differential Revision: https://secure.phabricator.com/D15662
Summary: Ref T7303. This application is currently stone-age tech (no transactions, hard "delete" action). Bring it up to modern specs.
Test Plan:
- Created and edited an OAuth application.
- Viewed transaction record.
- Tried to create something with no name, invalid redirect URI, etc. Was gently rebuffed with detailed explanatory errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7303
Differential Revision: https://secure.phabricator.com/D15609
Summary:
Ref T10538. Ref T10537. This creates PHIDs which represent GitHub users, and uses them as the actors for synchronized comments.
I've just made them Doorkeeper objects. There are three major kinds of objects they //could// possibly be:
- Nuance requestor objects.
- External account objects.
- Doorkeeper objects.
I don't think we actually need distinct nuance requestor objects. These don't really do anything right now, and were originally created before Doorkeeper. I think Doorkeeper is a superset of nuance requestor functionality, and better developed and more flexible.
Likewise, doorkeeper objects are much more flexible than external account objects, and it's nice to imagine that we can import from Twootfeed or whatever without needing to build full OAuth for it. I also like less stuff touching auth code, when possible.
Making these separate from external accounts does make it a bit harder to reconcile external users with internal users, but I think that's OK, and that it's generally desirable to show the real source of a piece of content. That is, if I wrote a comment on GitHub but also have a Phabricator account, I think it's good to show "epriestley (GitHub)" (the GitHub user) as the author, not "epriestley" (the Phabricator user). I think this is generally less confusing overall, and we can add more linkage later to make it clearer.
Test Plan:
{F1194104}
{F1194105}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537, T10538
Differential Revision: https://secure.phabricator.com/D15541
Summary:
Fixes T10234. This is a more thorough fix.
Root issue is that some time around D13589, we started hitting an object cache for `loadCustomInlineRules()`, but didn't adjust the code to account for that.
So if a page created multiple similar engines, we'd return the same `$rule` object for multiple engines, call `setEngine()` on it with different engines, and then possibly try to render using an already-expired engine the second time through.
Instead, create a separate `$rule` object for each separate `$engine`.
Test Plan:
Repro is something like this:
- Create a custominlinerule which uses an engine.
- Purge the remarkup cache.
- Load a page which uses the rule in two engines (e.g., in a revision description, and also in an inline comment).
- Before change: second one could fatal. After change: clean load.
Reviewers: thoughtpolice, chad
Reviewed By: thoughtpolice, chad
Subscribers: thoughtpolice, eadler
Maniphest Tasks: T10234
Differential Revision: https://secure.phabricator.com/D15535
Summary: Ref T10677, Awarding/revoking badge should create a feed story on homepage with badge handle recipient handles
Test Plan: Award/revoke badge, open Feed, should see story with badge link and recipient links.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10677
Differential Revision: https://secure.phabricator.com/D15534
Summary: Ref T10677, awarding/revoking a badge should create timeline entries with titles that are more clear (excludes homepage feed stories)
Test Plan: Award/revoke a badge to single or multiple users. See timeline entries that reflect those actions.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T10677
Differential Revision: https://secure.phabricator.com/D15533
Summary:
Ref T10537. For Nuance, I want to introduce new sources (like "GitHub" or "GitHub via Nuance" or something) but this needs to modularize eventually.
Split ContentSource apart so applications can add new content sources.
Test Plan:
This change has huge surface area, so I'll hold it until post-release. I think it's fairly safe (and if it does break anything, the breaks should be fatals, not anything subtle or difficult to fix), there's just no reason not to hold it for a few hours.
- Viewed new module page.
- Grepped for all removed functions/constants.
- Viewed some transactions.
- Hovered over timestamps to get content source details.
- Added a comment via Conduit.
- Added a comment via web.
- Ran `bin/storage upgrade --namespace XXXXX --no-quickstart -f` to re-run all historic migrations.
- Generated some objects with `bin/lipsum`.
- Ran a bulk job on some tasks.
- Ran unit tests.
{F1190182}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15521
Summary:
Ref T10537. These are objects which are bound to some external object, like a Maniphest task which is a representation of a GitHub issue.
This doesn't do much yet and may change, but my thinking is:
- I'm putting these on-object instead of on edges because I think we want to actively change the UI for them (e.g., clearly call out that the object is bridged) but don't want every page to need to do extra queries in the common case where zero bridged objects exist anywhere in the system.
- I'm making these one-to-one, more or less: an issue can't be bridged to a bunch of tasks, nor can a bunch of tasks be bridged to a single issue. Pretty sure this makes sense? I can't come up with any reasonable, realistic cases where you want a single GitHub issue to publish to multiple different tasks in Maniphest.
- Technically, one type of each bridgable object could be bridged, but I expect this to never actually occur. Hopefully.
Test Plan: Ran storage upgrade, loaded some pages.
Reviewers: chad
Reviewed By: chad
Subscribers: Luke081515.2
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15502
Summary: Fixes T10234. This usage is unusual, out of date, and has some bad interactions with engines and custom rules.
Test Plan:
- Added `CustomInlineCodeRule` from P1129 as an extension rule.
- Put a custom `<code> ... </code>` block in a Maniphest task description.
- Saw fatal as described in task; applied change; saw rule work properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10234
Differential Revision: https://secure.phabricator.com/D15501
Summary:
adds the `{{PHID....}}` rule. Should mostly be useful in UI code that refers to Objects.
It doesn't add any mention links/transactions.
Test Plan: Comment with this, see email (plain + html) and comment box.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15488
Summary:
First pass at converting Differential, I likely have some buggy-poos but thought I'd toss this up now in case very bad bugs present.
To do:
- Need to put status back on Hovercards
- "Diff Detail" probably needs a better design
Test Plan: Looking at lots of diffs, admittedly I dont have harbormaster, etc, running locally. Checked Diffusion for Table of Content changes on small and large commits.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15463
Summary: Ref T10537. Currently, when you have at least two cursors, the daemon can poll too frequently when processing the last source because it never hits the end-of-list condition.
Test Plan:
- Ran `bin/phd debug trigger`.
- Observed huge volumes of output before change as triggers fired as fast as possible.
- Observed reasonable poll frequency after change.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15464
Summary:
Ref T10563. This isn't a complete fix, but should make viewing complex inline threads a little more manageable.
This just tries to put stuff in thread order instead of in pure chronological order. We can likely improve the display treatment -- this is a pretty minimal approach, but should improve clarity.
Test Plan:
T10563 has a "before" shot. Here's the "after":
{F1169018}
This makes it a bit easier to follow the conversations.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10563
Differential Revision: https://secure.phabricator.com/D15459
Summary:
Fixes T10562. I left this behavior sort of ambiguous in the original implementation because I didn't anticipate or stumble across this situation.
It's easy to fix: when you reply to a ghost, just put the reply in the exact same place as the ghost (even if it's a different diff), so they always move/ghost/port/thread together.
Test Plan:
See T10562 for reproduction steps and a "before" picture. Here's the after picture:
{F1168983}
The two comments at the bottom are pre-fix, and exhibit the bug. The comment at the top is post-fix, and appears adjacent to the original correctly.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T10562
Differential Revision: https://secure.phabricator.com/D15458
Summary:
Ref T10537. More infrastructure:
- Put a `bin/nuance` in place with `bin/nuance import`. This has no useful behavior yet.
- Allow sources to be searched by substring. This supports `bin/nuance import --source whatever` so you don't have to dig up PHIDs.
Test Plan:
- Applied migrations.
- Ran `bin/nuance import --source ...` (no meaningful effect, but works fine).
- Searched for sources by substring in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15436
Summary:
Ref T10537. Some sources (like the future "GitHub Repository" source) need to poll remotes.
- Provide a mechanism for sources to emit import cursors.
- Hook them into the trigger daemon so they'll fire periodically.
- Provide some storage.
This diff does nothing useful or interesting, and is pure infrastructure.
Test Plan:
- Ran `bin/storage upgrade -f`, no adjustment issues.
- Poked around Nuance.
- Ran the trigger daemon, verified it didn't crash and checked for Nuance stuff to do.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10537
Differential Revision: https://secure.phabricator.com/D15435
Summary:
Two minor changes here:
- Replace `get/setUser()` with `get/setViewer()` for consistency with everything else.
- `getViewer()` now throws if no viewer is set. We had a lot of code that either "should" check this but didn't, or did check it in an identical way, duplicating work. In contrast, very little code checks for a viewer but works if one is not present.
Test Plan:
- Grepped for `->user`.
- Attempted to fix all callsites inside `*View` classes.
- Browsed around a bunch of applications, particularly Calendar, Differential and Diffusion, which seemed most heavily affected.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15412
Summary:
Every caller returns `true`. This was added a long time ago for Projects, but projects are no longer subscribable.
I don't anticipate needing this in the future.
Test Plan: Grepped for this method.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15409
Summary:
Ref T10457. Fixes T10024. This primarily just modernizes blueprints to use EditEngine.
This also fixes T10024, which was an issue with stored properties not being flagged correctly.
Also slightly improves typeaheads for blueprints (more information, disabled state).
Test Plan:
- Created and edited various types of blueprints.
- Set and removed limits.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10024, T10457
Differential Revision: https://secure.phabricator.com/D15390
Summary: Ref T10394. Currently, these rules are only active if the Macro application is installed. Instead, install them unconditionally.
Test Plan:
- Used `{icon camera}` with Macro installed and uninstalled.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10394
Differential Revision: https://secure.phabricator.com/D15311
Summary: Moves all the one off object calls to PHUIRemarkupView, adds a "Document" call as well (future plans).
Test Plan: Visited most pages I could get access to, but may want extra careful eyes on this diff.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15281
Summary: Clean the UI up a little.
Test Plan: {F1106533}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D15259
Summary:
Fixes T10330.
- Anywhere we support "matches regexp", also allow "does not match regexp". Although you can sometimes write a clever negative regexp, these rules are better expressed with "does not match <simple regexp>" anyway, and sometimes no regexp will work.
- Always allow "does not contain" when we support "contains".
- Fix some JS issues with certain rules affecting custom fields.
Test Plan:
- Wrote an "Affected files do not match regexp" rule that required every diff to touch "MANUALCHANGELOG.md".
- Tried to diff without the file; rejected.
- Tried to diff with the file; accepted.
- Wrote a bunch of "contains" and "does not contain" rules against text fields and custom fields, then edited tasks to trigger/observe them.
- Swapped the editor into custom text, user, remarkup, etc fields, no more JS errors.
{F1105172}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10330
Differential Revision: https://secure.phabricator.com/D15254
Summary: I can't find any reference to these used. Fixes T10244
Test Plan: Grep for "phui-text" and "PHUI::TEXT"
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T10244
Differential Revision: https://secure.phabricator.com/D15142
Summary: Mostly for consistency, we're not using other forms of icons and this makes all classes that use an icon call it in the same way.
Test Plan: tested uiexamples, lots of other random pages.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D15125
Summary: Fixes T8762.
Test Plan: Ran `bin/storage upgrade --namespace ... --user limited`, saw a more specific error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8762
Differential Revision: https://secure.phabricator.com/D15080
Summary:
Ref T10195. Distinguish between "database does not exist" and "database exists, you just don't have permission to access it".
We can't easily get this information out of INFORMATION_SCHEMA but can just `SHOW TABLES IN ...` every database that looks like it's missing and then look at the error code.
Test Plan:
- Created a user `limited` with limited access.
- Ran `bin/storage adjust`.
- Got hopefully more helpful messages about access problems, instead of "Missing" errors.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10195
Differential Revision: https://secure.phabricator.com/D15079
Summary:
Ref T10188. If you issue certain queries which use reverse ordering (like "All tasks, oldest update to newest update") and then try to page forward, we build the paging clause without reversing the column order correctly.
For example, the ordering of "oldest update to newest update" is "dateModified ASC, id ASC", so the second page should include an "id > X" query. Currently, this builds as "id < X" incorrectly instead.
The cause of this is just a failure to re-reverse a reversing flag when constructing the paging clause.
Test Plan:
- Queried tasks by update, oldest to newest, with no grouping, etc.
- Paged to second page.
- After change, got a valid second page with a good query in the Services tab.
- Made some other normal queries.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10188
Differential Revision: https://secure.phabricator.com/D15076
Summary:
Ref T6183. Ref T10054. Historically, only members could watch projects because there were some weird special cases with policies. These policy issues have been resolved and Herald is generally powerful enough to do equivalent watches on most objects anyway.
Also puts a "Watch Project" button on the feed panel to make the behavior and meaning more obvious.
Test Plan:
- Watched a project I was not a member of.
- Clicked the feed watch/unwatch button.
{F1064909}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6183, T10054
Differential Revision: https://secure.phabricator.com/D15063
Summary:
Fixes T10135. When the viewer is a member of no projects, specify the constraint type as a new "EMPTY" type.
When a query has an "EMPTY" constraint, fail fast with no results.
Test Plan:
- Viewed a viewerprojects() query result set as a user in no projects.
- Before patch: got a lot of hits. After patch: no hits.
- Viewed a normal result set, no changes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10135
Differential Revision: https://secure.phabricator.com/D15003
Summary: See Q266.
Test Plan: Created a bulk job, clicked "Details" instead of "Confirm", clicked "Continue" to get back to confirmation dialog.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14985
Summary:
Ref T10010. I think this is the desired/expected default behavior (e.g., searching for "Maniphest" should find tasks in any subproject or sprint of that project).
I'll probably add an "exact(...)" function later to mean "only the Maniphest superproject, exactly, not any of its children".
Test Plan:
- Added and executed unit tests.
- Ran various queries from the web UI.
- Got sensible-seeming results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14910
Summary: Ref T10010. This is pretty straightforward with a couple of very minor new behaviors, like the icon selector edit field.
Test Plan:
- Created projects.
- Edited projects.
- Saw "Create Project" in quick create menu.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10010
Differential Revision: https://secure.phabricator.com/D14896
Summary: Ref T8980. Previously, if you had like `T123T123T123` and waved your mouse over them, we wouldn't move the card. Now, move the card.
Test Plan: Waved mouse. Saw the card move.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14881
Summary:
Ref T8980. Move away from events to EngineExtensions.
This also simplifies hovercards a bit:
- Removes tasks from revision cards.
- Removes blockers/blocked from task cards.
- Removes "Send Message" from user cards.
These mostly felt cluttery to me. Open to arguments to retain them. I think we can make better use of the space, though (e.g., flags, projects + board columns).
Test Plan:
- Viewed people, task, revision, commit and project hovercards.
{F1043256}
{F1043257}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14878
Summary:
Ref T8980. This isn't 100% coverage but should be pretty much all of the common ones.
These feel a touch iffy to me at first glance so I didn't go crazy trying to hunt all of them down. I have some other plans for them so maybe they'll feel better by the end of it.
Test Plan: Hovered over author, reviewers, blocked tasks, projects, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8980
Differential Revision: https://secure.phabricator.com/D14877
Summary:
After certain types of errors, we may deadlock when trying to destroy test databases.
Specifically, we still have connections open to, say, `phabricator_unittest_abasonaknlbaklnasb_herald` (or whatever) and MySQL sometimes (not sure exactly when?) waits for them before destorying the database.
Test Plan:
- Added `$m = null; $m->method()` to a fixture test to force a fatal.
- Saw consistent deadlock, with `storage destroy` never exiting.
- Added `--trace` to the `storage destroy` command and made it use `phutil_passthru()` so I could see what was happening.
- Saw it hang on some arbitrary database.
- Conneced to MySQL, used `show full processlist;` to see what was wrong.
- Saw the `DROP DATABASE ...` command waiting for locks to release on the database, and other connections still open.
- Applied patch.
- Saw consistent success.
- Used `storage destroy --unittest-fixtures` to clean up extra databases.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D14875
Summary: Ref T8509. We currently give you a fairly obtuse error when trying to name a project something like "!!". The error is correct, but not as helpful as it could be. Give users a more specific, more helpful error.
Test Plan: {F1042883}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8509
Differential Revision: https://secure.phabricator.com/D14872
Summary:
Ref T8509. This fixes three issues:
- Adding a slug like `UPPERCASE` would not give you a normalized slug. (Now: normalizes as `uppercase`.)
- Adding a slug like `UPPERCASE` would allow you to give two different projects the different tags `UPPERCASE` and `uppercase` (and `UpPeRcAsE`, etc). (Now: second tag is rejected as a duplicate.)
- Adding multiple identical or similar slugs would produce a duplicate key exception. (Now: ignores the duplicates.)
Test Plan:
- Added test coverage.
- Made tests pass.
- Hit these cases in the UI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8509
Differential Revision: https://secure.phabricator.com/D14870
Summary:
Ref T9979. This uses ngrams (specifically, trigrams) to build a reasonably efficient index for substring matching. Specifically, for a package like "Example", with ID 123, we store rows like this:
```
< ex, 123>
<exa, 123>
<xam, 123>
<amp, 123>
<mpl, 123>
<ple, 123>
<le , 123>
```
When the user searches for `exam`, we join this table for packages with tokens `exa` and `xam`. MySQL can do this a lot more efficiently than it can process a `LIKE "%exam%"` query against a huge table.
When the user searches for a one-letter or two-letter string, we only search the beginnings of words. This is probably what they want, the only thing we can do quickly, and a reasonable/expected behavior for typeaheads.
Test Plan:
- Ran storage upgrades and search indexer.
- Searched for stuff with "name contains".
- Used typehaead and got sensible results.
- Searched for `aabbccddeeffgghhiijjkkllmmnnooppqqrrssttuuvvwwxxyyzz` and saw only 16 joins.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14846
Summary:
Ref T9979. This event had one weird callsite and no known third-party callers. It can be done more cleanly as an extension, now.
This index is used to allow us to "Group By: Project" in Maniphest without joining into the Projects database.
Test Plan:
- Ran a query with "Group By: Project" in Maniphest.
- Renamed project "Apples" to "Zebras".
- Reloaded page.
- UI properly moved "Zebras" tasks to the bottom of the list.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14836
Summary: Ref T9979. This is going to become `FulltextEngine`, but pave the way for that by pulling extensions out of it.
Test Plan:
{F1036624}
- Used `bin/search index Txxx`, saw projects, subscribers and custom fields rebuild in the index.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14835
Summary: Ref T9979. Convert all DestructionEngine behaviors to extensions.
Test Plan:
{F1033244}
Destroyed an object, verifying:
- Herald transcripts were destroyed;
- edges were destroyed;
- flags were destroyed;
- tokens were destroyed;
- transactions were destroyed;
- worker tasks were cancelled.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9979
Differential Revision: https://secure.phabricator.com/D14832
Summary: Ref T10004. I missed these previously, so they didn't work quite right. Restore them to glory.
Test Plan: Edited remarkup and text custom fields on an Owners package.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10004
Differential Revision: https://secure.phabricator.com/D14821
Summary:
Ref T9994. This fixes the first issue discussed on that task, which is that when a merge fails after "arc land", we would not clean up all the leases properly.
Specifically, when a merge fails, we use `queueTask()` to schedule a followup task. This followup destroys the lease and frees the underlying resource.
However, the default behavior of `queueTask()` is to //not queue tasks// if the parent task fails. This is a reasonable, safe behavior that was originally introduced in D8774, where it kept us from sending too much mail if a task did "send some mail" and then failed a little later on and got retried.
Since I think the default behavior is correct, I just special cased the behavior for Drydock to make it queue even on failure. These are the only types of followup tasks we currently want to queue on main task failure.
(It's possible that future Blueprints might want some kind of more specialized behavior, where some tasks queue only on success, but we can cross that bridge when we come to it.)
Test Plan:
- See T9994#149878 for test case setup.
- I ran that test case again with this patch, and saw the followup task queue properly in the `--trace` log, a correspoinding update task show up in `/daemon/`, and the lease get destroyed when I ran it a moment later.
{F1029915}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9994
Differential Revision: https://secure.phabricator.com/D14818
Summary:
Ref T10004. Fixes T5158. There was a long-standing issue with defaults not working properly, but EditEngine has made it more obvious because it's a lot easier to set defaults now.
The issue is basically that the defaults are getting set as the field's real value early on, so when we go to generate the transaction "old value" later, we build a transaction that uses the //new// value as both the "new value" and "old value". Then the engine says "you didn't change anything, so I'm going to ignore this" and drops it.
To fix this, return `null` as the "old value" by default, and add a call to overwrite that after we load a legitimate old value.
This fix is a touch iffy, but I have some grand plans to clean up the CustomField stuff more broadly later on.
Test Plan:
- Set config defaults on select/typeahead fields, created and edited tasks.
- Set form defaults on select/typehaead fields, created and edited tasks.
- In all cases, transactions and state accurately reflected edits.
- Set defaults on //hidden// fields, verified forms respected them correctly.
- This does generate some fluffy transactions, but I'll deal with those in T7661.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5158, T10004
Differential Revision: https://secure.phabricator.com/D14809
Summary:
Fixes T6864. This creates a sort of busy menu but I think that's proably fine -- users are opting into activating these fields for search anyway.
In the future, we could refine this as, e.g.:
- don't show these options in the dropdown;
- do show them on some new "http prefilling" sort of page;
- then you access them as an advanced user with `?order=secret-magic`.
But I'm not going to bother for now.
Test Plan: Ordered by an int field, then reversed the order.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T6864
Differential Revision: https://secure.phabricator.com/D14800
Summary:
Ref T9964. Three goals here:
- Make it easier to supply Conduit documentation.
- Make automatic documentation for `*.edit` endpoints more complete, particularly for custom fields.
- Allow type resolution via Conduit types, so you can pass `["alincoln"]` to "subscribers" instead of needing to use PHIDs.
Test Plan:
- Viewed and used all search and edit endpoints, including custom fields.
- Used parameter type resolution to set subscribers to user "dog" instead of "PHID-USER-whatever".
- Viewed HTTP parameter documentation.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14796
Summary: Ref T9964. This just adds more structure to application fields, to make it harder to make typos and easier to validate them later.
Test Plan: Viewed APIs, called some APIs, saw good documentation and correct results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14776
Summary:
Ref T9964. Fill in more parameter types and descriptions.
(No date support yet since it's a bit more involved.)
Test Plan: {F1024022}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14766
Summary:
Ref T9964. ApplicationSearch currently has a bunch of hard-coded `if ($object instanceof thing)` stuff.
Pull that out so it can live in extensions.
Test Plan:
- Searched by spaces, subscribers, projects.
{F1023921}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14764
Summary:
Ref T9964. Adds a new-style "owners.search" endpoint, and an extension for customfields.
Puts enough indirection in place to give us nice, consistent "custom.key" user-facing keys instead of "std:custom:owners:na0shf9a8dfdsafl" junk.
Test Plan:
- Searched Owners via API.
- Searched by ID.
- Ordered by custom fields.
- Reviewed API docs.
- Used normal search with ordering.
- Viewed custom field values in search results.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9964
Differential Revision: https://secure.phabricator.com/D14758
Summary:
Fixes T9945. This is straightforward.
The two sub-object types are very lightweight so I just deleted them directly instead of loading + delete()'ing (or implementing DestructibleInterface on them, which would require they have PHIDs).
Also improve a US English localization.
Test Plan:
- Used `bin/remove destroy PHID-... --trace` to destroy a package.
- Verified it was gone.
- Inspected the SQL in the log for general reasonableness.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9945
Differential Revision: https://secure.phabricator.com/D14729
Summary: Fixes T9919. We were missing feed strings and US English localizations for some of this stuff.
Test Plan:
Before:
{F1018877}
After:
{F1018879}
{F1018880}
{F1018881}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9919
Differential Revision: https://secure.phabricator.com/D14721
Summary: Fixes T9851. I'll hold this for a while to give users some time to update per T9860.
Test Plan:
Edited a task via:
- Conduit
- Comments field
- Edit form
- New task form
Reviewers: chad
Reviewed By: chad
Subscribers: Krenair
Maniphest Tasks: T9851
Differential Revision: https://secure.phabricator.com/D14576
Summary:
Fixes T9920. When hiding changes, tell users why so they can learn the comment rule (usually, "Changes from before your most recent comment are hidden."; sometimes they're hidden for pagination reasons).
Also use "Show Older Comments" instead of "Show older comments." for the action since I think it's a little more consistent to use title case for links/actions?
Test Plan:
- Viewed a task with a lot of comments, saw a "most recent comment" element.
- Artificially set page size to 3, saw a "lots of changes" hide.
- Grepped for removed string.
- Clicked both "show older stuff" links.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T9920
Differential Revision: https://secure.phabricator.com/D14719
Summary:
Ref T9132. Ref T9908. Fixes T5622. This allows you to copy some fields (projects, subscribers, custom fields, some per-application) from another object when creating a new object by passing the `?template=xyz` parameter.
Extend "copy" support to work with all custom fields.
Test Plan:
- Created new pastes, packages, tasks using `?template=...`
- Viewed new template docs page.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T5622, T9132, T9908
Differential Revision: https://secure.phabricator.com/D14699
Summary:
Ref T9132. When I've touched `PhabricatorApplication` I keep hitting this bad `pht()` junk.
The warning is correct, these strings are not extactable and can not be translated.
Fix it so they can be extracted and translated.
Broadly, in all cases we want to render one of these:
> 95 Things (for fewer than some limit)
> 99+ Things (when we hit the limit)
Test Plan: Looked at homepage status counts, moused over them, saw reasonable strings. Grepped for removed method.
Reviewers: chad
Reviewed By: chad
Subscribers: joshuaspence
Maniphest Tasks: T9132
Differential Revision: https://secure.phabricator.com/D14638
