Summary:
Ref T8455. Begins consolidating the code for applying these effects:
- Makes Add/Remove subscribers a standard effect, and uses it in Pholio.
- This includes the "don't re-subscribe users who have explicitly unsubscribed" logic from Differential in the standard effect. I think this rule is always desirable.
- This adds new filtering of invalid PHID types to resolve the `arc diff` issue in T8455 once Differential uses this standard effect.
- Added "Remove Subscribers" to MockAdapter in order to test that it works.
- Relabeled "CC" in Pholio to "Subscribers" for consistency.
Test Plan:
- Created several rules which add subscribers to (and remove subscribers from) mocks.
- Updated mocks, changing properties and adding and removing subscribers.
- Observed transactions applying and aggregating properly.
- Observed add/remove rules each working correctly.
- Observed the "don't re-add unsubscribed users" condition acting on subscribers who had previously been added but explicitly removed/unsubscribed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13179
Summary:
Ref T8455. The Herald code in general isn't nearly as modular as it should be, and the subscriber code particularly has some legacy cruft. This is making it fragile and causing the issue described in T8455.
Currently, each Herald adapter has essentially identical code which it uses to determine which users are subscribed to an object. Instead, share code between object types.
I removed "explicitCCs":
- The value was always identical to doing the query in the common/standard way.
- They were only used to print a diagnostic message on transcripts, which I think is no longer relevant.
- I believe it predates transactions, so when it was added you couldn't figure out the old object state by looking at the transaction history. Now, CC changes are recorded there, so there's no need to restate the CC state on the transcript.
- Even if we do want to restore this (or something similar), we can do it directly from Herald now.
Test Plan:
- Created rules that use the "CCs" field in Herald, Pholio, Maniphest and Differential.
- Updated objects in each application.
- Observed valid field reads in the tranascript.
- Grepped for `FIELD_CC`.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8455
Differential Revision: https://secure.phabricator.com/D13177
Summary: Ref T8463.
Test Plan:
- Created a new revision via web UI with a username `@mention` in the summary and no repository.
- Prior to patch, hit a "not attached" error.
- After patch, no error.
- Created a new web UI revision, as above, but with a repository; saw repository work fine.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8463
Differential Revision: https://secure.phabricator.com/D13205
Summary: Tidying up of documentation, adding some new groups.
Test Plan: Eyeball it.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13133
Summary: Fixes T8433. Add an index to the `phabricator_feed.feed_storynotification` table. Primarily, this speeds up object destruction.
Test Plan: Ran `./bin/storage upgrade` and saw the migration apply cleanly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8433
Differential Revision: https://secure.phabricator.com/D13164
Summary: Remove the `*TransactionType` classes and define the constants in the corresponding `*Transaction` class instead.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13188
Summary: See D13185, the `testMethodVisibility()` test was not being executed.
Test Plan: `arc unit`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13186
Summary: Fixes T8458, Show informative errors when attempting to set a recurrence end date on a non-recurring event.
Test Plan: Create new event, set recurrence end date via date-picker without setting the "is recurring" checkbox, and attempt to save. Should get error saying there cannot be a recurrence end date on a non-recurring event.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8458
Differential Revision: https://secure.phabricator.com/D13192
Summary:
Ref T8441. Ref T7715.
- These are obsolete after the Viewer/HandlePool changes.
- These are unused after the typeahead parameterization changes.
Test Plan: `grep`, poked around.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13176
Summary: I saw these appear untranslated in the feed on https://secure.phabricator.com.
Test Plan: Created a dependency and checked the story text in `/feed/`.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13149
Ref T8454. This got gummed up in swapping between double negatives like "noDisabled".
Viewed queue, saw "Hide Disabled Users" instead of "Show Only Disabled Users".
Auditors: joshuaspence, btrahan
Summary: Ref T8441. Does what it says, provided other conditions (like using the new SearchField stuff) are fulfilled.
Test Plan:
{F473836}
{F473837}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8441
Differential Revision: https://secure.phabricator.com/D13171
Summary: Ref T8441. Ref T7715. Let SearchFields do some value massaging before buiding queries so we don't have to work as hard in each SearchEngine. Particularly, they can handle evaluateTokens() from Tokenizers.
Test Plan: Paste automatically gained access to `viewer()`, etc.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13170
Summary:
Ref T8441. Ref T7715. Add a layer of indirection to fields in search engines. This will allow us to:
- Simplify SearchEngine code, which has collected a lot of duplication around expressing what is effectively field types.
- Automatically add fields like "Spaces" and "Projects" (primary driver for T8441).
- Reorder or hide fields (not sure if we really want to do this, but it seems plausible, and this will let us play around with it, at least).
- Drive Conduit Query methods via SearchEngines, so the same code specifies both the search UI and the `application.query` endpoint (primary driver in T7715).
Test Plan:
- Searched for stuff in Paste, everything behaved exaclty like it used to (except that I removed the "no language" checkbox, which seemed like fluff from a bygone era).
- Searched for stuff in other applications, saw no changes.
- Hit date field errors.
- Used query strings to specify values.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7715, T8441
Differential Revision: https://secure.phabricator.com/D13169
Summary: Ref T8441. I want to use `PhabricatorSearchField` for a better, more useful object.
Test Plan: `grep`, `arc lint`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8441
Differential Revision: https://secure.phabricator.com/D13168
Summary:
Ref T8434. Hard-codes form sources as a complaint form.
This form is close to perfect and I'm not actually sure we need to let users customize it at all.
Test Plan: {F473587}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13166
Summary: Ref T8434. Throw these together.
Test Plan: Created a new Queue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13163
Summary: Ref T8434. Now you can list them.
Test Plan: Listed sources.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13162
Summary:
Ref T8434. Minor cleanup/modernization. I made type selection modal (like Herald, Auth, etc) so we can render the form on the next screen based on the type.
{F472519}
Test Plan: Created a new source, edited an existing source.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13161
Summary: Ref T8434. Touch things up a bit. This also cleans up the missing ApplicationTransactionInterface for T6367.
Test Plan: Didn't create a new queue. Didn't edit an existing queue.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: jeremyb, epriestley
Maniphest Tasks: T8434
Differential Revision: https://secure.phabricator.com/D13160
Summary:
Ref T8424. This adds crude integration with Paste's edit/view workflows: you can change the space a Paste appears in, see transactions, and get a policy callout.
Lots of rough edges and non-obviousness but it pretty much works.
Test Plan:
- Created and updated Pastes.
- Moved them between spaces, saw policy effects.
- Read transactions.
- Looked at feed.
- Faked query to return no spaces, saw control and other stuff vanish.
- Faked query to return no spaces, created pastes.
- Tried to submit bad values and got errors.
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13159
Test Plan: Submitted a form - saw nothing out of ordinary.
Reviewers: #blessed_reviewers
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13167
Summary: Ref T4558. Allow ghost atoms to be rendered in #diviner. This functionality didn't exist previously, but was hinted at by the TODO comments.
Test Plan: Generated #diviner documentation for rARC and then removed a class (before re-generating the documentation). Navigated to the documentation for the removed class and saw "This atom no longer exists".
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T4558
Differential Revision: https://secure.phabricator.com/D13114
Summary:
Ref T8424. No UI or interesting behavior yet, but integrates Spaces checks:
- `PolicyFilter` now checks Spaces.
- `PolicyAwareQuery` now automatically adds Spaces constraints.
There's one interesting design decision here: **spaces are stronger than automatic capabilities**. That means that you can't see a task in a space you don't have permission to access, //even if you are the owner//.
I //think// this is desirable. Particularly, we need to do this in order to exclude objects at the query level, which potentially makes policy filtering for spaces hugely more efficient. I also like Spaces being very strong, conceptually.
It's possible that we might want to change this; this would reduce our access to optimizations but might be a little friendlier or make more sense to users later on.
For now, at least, I'm pursuing the more aggressive line. If we stick with this, we probably need to make some additional UI affordances (e.g., show when an owner can't see a task).
This also means that you get a hard 404 instead of a policy exception when you try to access something in a space you can't see. I'd slightly prefer to show you a policy exception instead, but think this is generally a reasonable tradeoff to get the high-performance filtering at the Query layer.
Test Plan:
- Added and executed unit tests.
- Put objects in spaces and viewed them with multiple users.
- Made the default space visible/invisible, viewed objects.
- Checked the services panel and saw `spacePHID` constraints.
- Verified that this adds only one query to each page.
Reviewers: btrahan, chad
Reviewed By: btrahan
Subscribers: chad, epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13156
Summary:
Ref T8424. When users are rejected because they can't see the space an object is in, this isn't really a capability rejection. Don't require a capability when rejecting objects.
This mostly simplifies upcoming changes.
Test Plan:
- Viewed a capability exception dialog, it looked the same as always.
- (After additional changes, viewed a space exception dialog.)
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13155
Summary:
Ref T8424. I'm using Paste as a testbed application because Spaces make some degree of sense for it but it's also flat/simple.
This doesn't do anything interesting or useful and mostly just making the next (more interesting) diff smaller.
Test Plan:
- Ran `bin/storage upgrade -f`.
- Browsed pastes.
- Created a paste.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13154
Summary:
Ref T8424. This adds a standard KeyValueCache to serve as a request cache.
In particular, I need to cache Spaces (they are frequently accessed, sometimes by multiple viewers) but not have them survive longer than the scope of one request.
This request cache is explicitly destroyed by each web request and each daemon request.
In the very long term, building this kind of construct supports reusing PHP interpreters to run web requests (see some discussion in T2312).
Test Plan:
- Added and executed unit tests.
- Ran every daemon.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8424
Differential Revision: https://secure.phabricator.com/D13153
Summary:
Ref T8424. Fixes T7114. This was envisioned as a per-request cache for reusing interpreters, but isn't a good fit for that in modern Phabricator.
In particular, it isn't loaded by the daemons, but they have equal need for per-request caching.
Since I finally need such a cache for Spaces, throw the old stuff away before I built a more modern cache.
Also resolves T7114 by dropping filtering on $_SERVER. I'm pretty sure this is the simplest fix, see D12977 for a bit more discussion.
Test Plan: Called `didFatal()` from somewhere in normal code and verified it was able to use the access log.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7114, T8424
Differential Revision: https://secure.phabricator.com/D13152
Summary:
Fixes T8326. This removes calls to PhabricatorStartup from places that daemons may access.
This salt doesn't need to be global; it's embedded in the token we return. It's fine if we use a different salt every time. In practice, we always use the same viewer, so this change causes little or no behavioral change.
Ref T8424. For Spaces, I need a per-request cache for all spaces, because they have unusual access patterns and require repeated access, in some cases by multiple viewers.
We don't currently have a per-request in-process cache that we, e.g., clear in the daemons.
We do have a weak/theoretical/forward-looking attempt at this in `PhabricatorStartup::getGlobal()` but I'm going to throw that away (it's kind of junky, partly because of T8326) and replace it with a more formal mechanism.
Test Plan:
- Submitted some forms.
- Grepped for `csrf.salt`.
- Viewed page source, saw nice CSRF tokens with salt.
- All the salts are still the same on every page I checked, but it doesn't matter if this isn't true everywhere.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8326, T8424
Differential Revision: https://secure.phabricator.com/D13151
Summary: Fixes T7458. Integrates #diviner into #applicationsearch by indexing `DivinerLiveBook` and `DivinerLiveSymbol` search documents. Depends on D13157.
Test Plan: Ran `./bin/search index --all --type BOOK` and `./bin/search index --all --type ATOM` and then searched for various symbols via global search.
Reviewers: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7458
Differential Revision: https://secure.phabricator.com/D13090
Summary: I missed these in D13157.
Test Plan: Browsed around #diviner.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13158
Summary: Fixes T8401. Change `withIncludeGhosts()` to `withExcludeGhosts()` and `withIncludeUndocumentable()` to `withExcludeDocumentable()`. In particular, this allows querying for atoms by PHID to work as expected.
Test Plan: I got confused with double negatives so I might have gotten some of these wrong... I poked around Diviner and re-generated documentation to verify that this is working as expected.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8401
Differential Revision: https://secure.phabricator.com/D13157
This fixes this, which only crops up some of the time:
```
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] [2015-06-04 03:03:10] EXCEPTION: (PhutilProxyException) Error while executing Task ID 902271. {>} (PhabricatorDataNotAttachedException) Attempting to access attached data on PhabricatorRepositoryCommit (via getCommitData()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] arcanist(head=master, ref.master=8c589f1f759f), phabricator(head=master, ref.master=4a45620b0458), phutil(head=master, ref.master=afc05a9a7f00)
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #0 <#2> PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/repository/storage/PhabricatorRepositoryCommit.php:129]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #1 <#2> PhabricatorRepositoryCommit::getCommitData() called at [<phabricator>/src/applications/audit/editor/PhabricatorAuditEditor.php:669]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #2 <#2> PhabricatorAuditEditor::buildMailBody(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2178]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #3 <#2> PhabricatorApplicationTransactionEditor::sendMailToTarget(PhabricatorRepositoryCommit, array, PhabricatorMailTarget) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2152]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #4 <#2> PhabricatorApplicationTransactionEditor::sendMail(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:998]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #5 <#2> PhabricatorApplicationTransactionEditor::publishTransactions(PhabricatorRepositoryCommit, array) called at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:21]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #6 <#2> PhabricatorApplicationTransactionPublishWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #7 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:162]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #8 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:20]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #9 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:185]
Daemon 39827 STDE [Thu, 04 Jun 2015 03:03:11 +0000] #10 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
```
Auditors: btrahan
Fixes this, which only triggers on some kinds of mail:
```
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] [2015-06-04 02:56:38] EXCEPTION: (PhutilProxyException) Error while executing Task ID 902251. {>} (PhabricatorDataNotAttachedException) Attempting to access attached data on DifferentialRevision (via getActiveDiff()), but the data is not actually attached. Before accessing attachable data on an object, you must load and attach it.
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] Data is normally attached by calling the corresponding needX() method on the Query class when the object is loaded. You can also call the corresponding attachX() method explicitly. at [<phabricator>/src/infrastructure/storage/lisk/PhabricatorLiskDAO.php:166]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] arcanist(head=master, ref.master=8c589f1f759f), phabricator(head=master, ref.master=6dede2e2c513), phutil(head=master, ref.master=afc05a9a7f00)
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #0 <#2> PhabricatorLiskDAO::assertAttached(string) called at [<phabricator>/src/applications/differential/storage/DifferentialRevision.php:158]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #1 <#2> DifferentialRevision::getActiveDiff() called at [<phabricator>/src/applications/differential/customfield/DifferentialBranchField.php:72]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #2 <#2> DifferentialBranchField::updateTransactionMailBody(PhabricatorMetaMTAMailBody, DifferentialTransactionEditor, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2481]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #3 <#2> PhabricatorApplicationTransactionEditor::addCustomFieldsToMailBody(PhabricatorMetaMTAMailBody, DifferentialRevision, array) called at [<phabricator>/src/applications/differential/editor/DifferentialTransactionEditor.php:1208]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #4 <#2> DifferentialTransactionEditor::buildMailBody(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2178]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #5 <#2> PhabricatorApplicationTransactionEditor::sendMailToTarget(DifferentialRevision, array, PhabricatorMailTarget) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:2152]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #6 <#2> PhabricatorApplicationTransactionEditor::sendMail(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php:998]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #7 <#2> PhabricatorApplicationTransactionEditor::publishTransactions(DifferentialRevision, array) called at [<phabricator>/src/applications/transactions/worker/PhabricatorApplicationTransactionPublishWorker.php:21]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #8 <#2> PhabricatorApplicationTransactionPublishWorker::doWork() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorWorker.php:91]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #9 <#2> PhabricatorWorker::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php:162]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #10 <#2> PhabricatorWorkerActiveTask::executeTask() called at [<phabricator>/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php:20]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #11 PhabricatorTaskmasterDaemon::run() called at [<phutil>/src/daemon/PhutilDaemon.php:185]
Daemon 39819 STDE [Thu, 04 Jun 2015 02:56:38 +0000] #12 PhutilDaemon::execute() called at [<phutil>/scripts/daemon/exec/exec_daemon.php:125]
```
Auditors: btrahan
Summary: Ref T6367.
Test Plan:
- Added and executed unit tests.
- Sent mail to A (en_US) and B (en_A*).
- Got one mail in English and one mail in ENGLISH.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13142
Summary:
Ref T6367. Removes `multiplexMail()`!
We can't pass a single body into a function which splits it anymore: we need to split recipients first, then build bodies for each recipient list. This lets us build separate bodies for each recipient's individual translation/access levels.
The new logic does this:
- First, split recipients into groups called "targets".
- Each target corresponds to one actual mail we're going to build.
- Each target has a viewer (whose translation / access levels will be used to generate the mail).
- Each target has a to/cc list (the users who we'll ultimately send the mail to).
- For each target, build a custom mail body based on the viewer's access levels and settings (language prefs not actually implemented).
- Then, deliver the mail.
Test Plan:
- Read new config help.
Then did a bunch of testing, primarily with `bin/mail list-outbound` and `bin/mail show-outbound` (to review generated mail), `bin/phd debug taskmaster` (to run daemons freely) and `bin/worker execute --id <id>` (to repeatedly test a specific piece of code after identifying an issue).
With `one-mail-per-recipient` on (default):
- Sent mail to multiple users.
- Verified mail showed up in `mail list-outbound`.
- Examined mail with `mail show-outbound`.
- Added a project that a subscriber could not see.
- Verified it was not present in `X-Phabricator-Projects`.
- Verified it was rendered as "Restricted Project" for the non-permissioned viewer.
- Added a subscriber, then changed the object policy so they could not see it and sent mail.
- Verified I received mail but the other user did not.
- Enabled public replies and verified mail generated with public addresses.
- Disabld public replies and verified mail generated with private addresses.
With `one-mail-per-recipient` off:
- Verified that one mail is sent to all recipients.
- Verified users who can not see the object are still filtered.
- Verified that partially-visible projects are completely visible in the mail (this violates policies, as documented, as the best available compromise).
- Enabled public replies and verified the mail generated with "Reply To".
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: carlsverre, epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13131
Summary:
Ref T6367. Do all mail, feed, notification and search stuff from the daemons, in all editors.
There are four relatively-stateful editors (Audit, Differential, Phriction, PhortuneCart) which needed special care to move state into the daemons properly.
Beyond that, I moved mailTo/mailCC/feedRelated/feedNotify to be computed before we enter the worker:
- This is simpler, since a lot of editors rely on being able to call `$object->getReviewers()` or similar to compute them.
- This is more correct, since we want to freeze the lists at this moment in time.
Finally, I renamed `loadEdges` to `willPublish` and made it a slightly more general hook.
---
This is a bit fragile and I'm not //thrilled// about it.
It would probably be cleaner to have separate Editor and Publisher classes (something like @fabe's D11329 did). However, I think that's quite a lot of work, and I'd like to see stronger motivation for it (either in this actually being more fragile than I think, or there being other things we get out of it). Overall, I'm comfortable with this change, just definitely not a big fan of the "save" + "load" pattern since I think it's really fragile, nonobvious, hard to debug/predict, etc.
Test Plan:
Directly updated editors:
- Created a new Phriction page, saw "Document Content".
- Edited a Phriction page, saw "Document Diff".
- Edited a revision, got normal looking mail.
- Faked in `changedPriorToCommitURI` and verified it survived the state boundary.
- Sent Audit mail.
- Sent invoice mail.
Indirect editors - for these, I just made a change and made sure the mail generated:
- Updated a paste.
- Updated an event.
- Updated a thread.
- Updated a task.
- Updated a mock.
- Updated a question.
- Updated a project.
- Updated a file.
- Updated an initiative.
- Updated a Legalpad document.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, fabe
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13115
Summary:
Ref T6367. This is similar to D11329, but not quite as ambitious.
Allow Editors to implement `supportsWorkers()` and move their publishing work into a daemon. So far, only Paste supports this.
Most of the complexity here is saving and restoring state across the barrier between the web process and the worker process, but I think this is ~90% of it and then we'll pick up a couple of random things in applications.
I'm primarily trying to keep this as gradual as possible.
Test Plan:
- Published transactions with and without daemon support.
- Looked at mail, feed.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: fabe, epriestley
Maniphest Tasks: T6367
Differential Revision: https://secure.phabricator.com/D13107
Summary:
Ref T7703. See that task and inline for a bunch of discussion.
Briefly, when we run implicit policy rules ("to see a revision, you must also be able to see its repository") at query time, they don't apply to other viewers we might check later.
We do this very rarely, but when we do we're often doing it for a bunch of different viewers (for example, in Herald) so I don't want to just reload the object a million times.
Test Plan:
- Added and executed unit tests.
- Wrote a "flag everything" Herald rule, as in the original report in T7703, and no longer got "Unknown Object" flags on revisions.
- Rigged up a lot of cases in the web UI and couldn't find any inconsistencies, although this case is normally very hard to hit.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T7703
Differential Revision: https://secure.phabricator.com/D13104
Summary:
Fixes T8387. This completes conversion of lists into users.
These settings allow administrators to reduce the amount of mail delivered to lists, ahead of T5791.
Test Plan: {F468206}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13138
Summary: Ref T8387. Mostly completeness, but you might want to choose html vs text mail.
Test Plan: {F468203}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13137
Summary:
Ref T8387. Ref T6367. This allows selection of a language, which will be respected in email delievered to the list users.
For example, you could have a German list that gets mail in German or something. I don't know that the feature is really useful, it's mostly just for completeness.
I also supported it for bots, mostly so their pronouns can be configured.
Test Plan: {F468186}
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T6367, T8387
Differential Revision: https://secure.phabricator.com/D13136
Summary: Ref T8377. These were picked up by tests.
Test Plan: `arc unit --everything`
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8377
Differential Revision: https://secure.phabricator.com/D13130
Summary: Ref T8387. This is now completely obsoleted by mailing list users.
Test Plan: Grepped for `mailinglist` and related symbols.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13129
Summary: Ref T8387. Mostly just modernizes this panel to work for viewers vs users. Auto-verify these edits since they aren't otherwise verifiable.
Test Plan:
- Added, changed, removed addresses for a list.
- Used panel normally for my own account.
- Verified bots don't get a panel.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13125
Summary:
Ref T8387. Adds new mailing list users.
This doesn't migrate anything yet. I also need to update the "Email Addresses" panel to let administrators change the list address.
Test Plan:
- Created and edited a mailing list user.
- Viewed profile.
- Viewed People list.
- Searched for lists / nonlists.
- Grepped for all uses of `getIsDisabled()` / `getIsSystemAgent()` and added relevant corresponding behaviors.
- Hit the web/api/ssh session blocks.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, tycho.tatitscheff, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13123
Summary:
Ref T8387. This mostly merges D10565 + D10480. I'm going to touch this to add mailing list stuff shortly so I wanted to clean those up.
This isn't super pretty but is fully flexible and consistent with other modern query UIs.
This should be more-or-less backward compatible.
Test Plan: Fiddled with the new options.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: eadler, cburroughs, epriestley
Maniphest Tasks: T8044, T8387
Differential Revision: https://secure.phabricator.com/D13122
Summary:
Ref T8387. This describes changes I haven't made yet, but plan to make.
Also removes the long-deprecated actAsUser capability so I can remove the caveat about it from the documentation.
Test Plan: `grep`, reading
Reviewers: btrahan, eadler
Reviewed By: btrahan, eadler
Subscribers: eadler, epriestley
Maniphest Tasks: T8387
Differential Revision: https://secure.phabricator.com/D13120
Summary: This got removed by accident in D12933.
Test Plan: Editing names works now.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13144
Summary: When destroying an object, also remove all associated flags and tokens.
Test Plan: Awarded a Maniphest task with a token and flagged it as well. Destroyed it with `./bin/remove destroy` and saw flag and token removed from the database.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13112
Summary:
This simply adds colors, icons to ProjectQueryConduitAPIMethod and
ProjectCreateConduitAPIMethod.
Additionally, adds 'tags' to ProjectCreateConduitAPIMethod
Change-Id: Ic6332fe174a59ecfd60cea281ccb0ed938136141
Test Plan:
create a new project with project.create, specify icon, color and tags
call project.query with colors and icons args, check for results
containing matching projects.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: legoktm, aklapper, Korvin, epriestley, valhallasw, qgil
Differential Revision: https://secure.phabricator.com/D13098
Summary: Closes T8371, Query for recurring event parents should accept a "not a child of anything" clause
Test Plan: querying for a list of events with recurring events should still work the same.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8371
Differential Revision: https://secure.phabricator.com/D13140
Summary: This class is no longer used since D10792.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13116
Summary: Ref T8371, Cancelled recurring events should propogate to real child events
Test Plan: Create recurring event, create and exception to a ghost event, cancel recurring event, real ghost event should be treated as cancelled while the recurring event remains cancelled.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8371
Differential Revision: https://secure.phabricator.com/D13121
Summary: Ref T8394, List view should only allow 100 ghost events per recurring event
Test Plan: Open list view with many recurring events. Shouldn't hang.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8394
Differential Revision: https://secure.phabricator.com/D13132
Summary: This class is no longer used.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13117
Summary: This class is no longer used after D13032.
Test Plan: `grep`
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13118
Summary: This class is no longer used after D12850.
Test Plan: `grep`
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13119
Summary: Fixes T8382, Edit link of an exception to a ghost event should not create new event
Test Plan: Create recurring event, open ghost, edit, save, edit again. Should edit event created after first edit, not new event.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8382
Differential Revision: https://secure.phabricator.com/D13108
Summary: Now that `ManiphestTask` implements `PhabricatorApplicationTransactionInterface`, the transaction removal happens automatically.
Test Plan: Used `./bin/remove destroy` to delete a `ManiphestTask` and saw related transactions removed as well.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13110
Summary: Ref T8357, DRAFT, recurring events need optional end dates
Test Plan: Edit recurring event, set end date, save, recurring ghosts should not generate after end date
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8357
Differential Revision: https://secure.phabricator.com/D13088
Summary: got some errors in my sandbox when I updated. this fixes them and I think is the right naming convention.
Test Plan: /settings/panel/emailpreferences/ started working again! /spaces/ also loaded unit tests passed without errors
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8377
Differential Revision: https://secure.phabricator.com/D13103
Summary:
Fixes T8372. We currently store the entire resource path in the cachekey; shorten it to a maximum of 64 characters.
The key size is 128 and T8372 had an example of a 112-byte key which could run over 128 with a slightly-long cache namespace.
Test Plan:
- Not 100% sure this is the issue, but it seems likely.
- Dumped the cachekey before and after the change.
- Saw values like `celerity:/res/1432859262T/phabricator/e2460e8f/core.pkg.css-6eGjUCJORU3j` now.
Reviewers: btrahan, chad
Reviewed By: chad
Subscribers: epriestley
Maniphest Tasks: T8372
Differential Revision: https://secure.phabricator.com/D13095
Summary: Fixes T7603. This application is no longer used.
Test Plan: Went to `/repository/` and got a 404.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley
Maniphest Tasks: T7603
Differential Revision: https://secure.phabricator.com/D12734
Summary: Fixes T8369, Both ghost instances and exceptions to generated ghosts should be editable
Test Plan: Create recurring event, open /E{id}/1, edit, save. New exception should not be "recurring", and should be editable, as the regular ghost events.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8369
Differential Revision: https://secure.phabricator.com/D13089
Summary: Ref T7604. Remove these "arcanist project" fields from the `LiskDAO` classes, but leave the data intact (there are no more read/writes to this data).
Test Plan:
- Grepped for calls to these methods.
- Ran `./bin/storage adjust` to check for schema issues.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7604
Differential Revision: https://secure.phabricator.com/D13012
Summary: I think the language here is inverted? On a commit which is reverted, the text states `$author added a reverted commit: $commit`. In reality, `$commit` is the //reverting// commit, not the //reverted// commit.
Test Plan: Think about English.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13067
Summary: Closes T8358, Edit recurring ghost events, not recurring event.
Test Plan: Create recurring event, original event should not be editable, but ghost events should be editable.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8358
Differential Revision: https://secure.phabricator.com/D13075
Summary: Fixes T8354. Adds a class to each `th` and `td` for specificity.
Test Plan: Write a large table as an inline comment, see it wraps property on smaller screens.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8354
Differential Revision: https://secure.phabricator.com/D13076
Summary: If a host artifact exists in a Harbormaster build, where the Drydock lease no longer exists, then an error will be raised because of the attempt to access an undefined index. This changes the code to use `idx()` so that it correctly returns null instead.
Test Plan: Tested in production.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: joshuaspence, Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13074
Summary:
Ref T7984. With this, an install can add an ExternalSymbolsSource to src/extensions, which will include whatever
source they have.
Test Plan: search for php and python builtins.
Reviewers: joshuaspence, epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T7984
Differential Revision: https://secure.phabricator.com/D13036
Summary: Using `##` can cause some formatting issues, see D13071.
Test Plan: See D13071.
Reviewers: epriestley, #blessed_reviewers, chad
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D13072
Summary: Fixes T8347, Generated ghost events should start generating as early as the range start minus the duration of the event.
Test Plan: Create a year long weekly event, a year later, each day should have about 52 events.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8347
Differential Revision: https://secure.phabricator.com/D13069
Summary: Closes T8356, Ghost events should provide a link to original event
Test Plan: Open an instance at /calendar/event/{id}/{index}. It should have a link to /calendar/event/{id}. Original recurring event should not link to itself.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8356
Differential Revision: https://secure.phabricator.com/D13068
Summary: Fixes T8329. I was able to figure out a reasonable way to have the full conpherence default to the email settings panel. I think this is cleaner than making things a dialogue as I rambled about in the description for T8329.
Test Plan: using /bin/mail to verify correct email links were generated for conpherence notifications and maniphest (general) notifications.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8329
Differential Revision: https://secure.phabricator.com/D13058
Summary: Fixes T7696.
Test Plan: hit the quote button with no text - it worked. highlighted some text and hit the quote button - it worked. hit the list item button with no text - it worked. hit the list item button with text selected - it worked
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T7696
Differential Revision: https://secure.phabricator.com/D13060
Summary:
Fixes T8154. Now you can tell when a Conpherence was started by mousing over the date. I think this is sufficient.
This ends up cleaning up the code significantly; it used to inherit from some transaction view class and had built up lots of cludge over time. Much cleaner now.
Test Plan: moused over tooltips in both durable column and full conpherence and saw correct full dates. clicked on a timestamp in durable column and went to right place in full conpherence.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8154
Differential Revision: https://secure.phabricator.com/D13057
Summary: Fixes T8334, Month views with two instances of the same month day number highlight both as 'today'.
Test Plan: Open May, only today 27th should be highlighted
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T8334
Differential Revision: https://secure.phabricator.com/D13044
Summary: These could cause F442758.
Test Plan: {F442779}
Reviewers: btrahan, joshuaspence
Reviewed By: joshuaspence
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D13042
Summary: Ref T8026, Create button should be a dropdown with public and private options
Test Plan: Create both a public and private event. Confirm view policies apply correctly.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T8026
Differential Revision: https://secure.phabricator.com/D13033
Summary:
Ref T8320. Fixes T8317. Fixes T2831. Fixes T8073. Fixes T7127.
There was a bug with this line:
for ($ii = 0; $ii < count($paths); $ii++) {
...because the array may be sparse if there have been deletes, so `count($paths)` might be 3, but the real keys could be `1`, `5` and `6`. I think this was the primary issue behind T7127.
The old Editor did a lot of work to try to validate paths. When a path failed to validate, it silently discarded it. This was silly and pointless: it's incredibly bad UX; and it's totally fine if users saves "invalid" paths. This was likely the cause of T8317, and probably the cause of T8073.
T2831 I'm less sure about, but I can't reproduce it and I rewrote all the logic so I suspect it's gone.
This also records and shows edits, so if stuff does keep happening it should be more clear what's going on.
I removed some adjacent stuff:
- I removed the ability to delete packages. I'll add "disable" in a future diff, plus `bin/remove destroy`, like other objects. Getting rid of this now let me get rid of all the mail stuff.
- I removed "path validation" where packages would try to automatically update in response to commits. This doesn't necessarily make sense in Git/Mercurial, is sketchy, could easily have been the source of T2831, and seems generally complicated and not very valuable. We could maybe restore it some day, but I'd like to get Owners stable before trying to do crazy stuff like that.
Test Plan: {F437687}
Reviewers: chad, btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T8317, T8073, T7127, T2831, T8320
Differential Revision: https://secure.phabricator.com/D13032
