Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
This patch also fixes another similar strlen() occurrence in the same source file.
```
ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/applications/herald/management/HeraldTestManagementWorkflow.php:33]
```
Test Plan: Run `../phorge/bin/herald test`: Get no `strlen()` error anymore but only expected output `Usage Exception: Specify an object to test rules for with "--object".`
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25586
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
ERROR 8192: strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [/var/www/html/phorge/phorge/src/applications/herald/management/HeraldRuleManagementWorkflow.php:36]
```
Test Plan: Run `../phorge/bin/herald rule`: Get no `strlen()` error anymore but only expected output `Usage Exception: Specify a rule to edit with "--rule <id|monogram>".`
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25585
Summary:
`::class` is available since PHP 5.5 (5.5 is a minimal requirement by Phorge): https://www.php.net/manual/en/language.oop5.basic.php#language.oop5.basic.class.class
It makes finding code using IDEs easier; see discussion in D25500.
Thus replace all string return values with returning the `::class` constant instead, with one exception: 'PhabricatorSettingApplication' in `PhabricatorUserPreferencesSearchEngine.php` does not exist and makes arc lint fail so this string remained unchanged.
Also note that two occurrences were wrapped in `pht()` for reasons I do not know.
List of functions whose return value get updated in this code change:
* getApplicationClassName()
* getAdapterApplicationClass()
* getDatasourceApplicationClass()
* getEditorApplicationClass()
* getEngineApplicationClass()
* getPHIDTypeApplicationClass()
* getQueryApplicationClass()
cf. T15158
Test Plan: Too broad - click around, basically.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15158
Differential Revision: https://we.phorge.it/D25524
Summary:
Add `getQueryApplicationClass()` to all `*TransactionQuery.php` classes similar to other `*Query.php` classes having the same function, and make the parent function in `PhabricatorApplicationTransactionQuery.php` abstract.
In the future, this will enable excluding transaction query results based on their underlying application (for example if an application has been uninstalled) to mitigate the problem of overheated search results. See https://we.phorge.it/T15642 for context.
The only callers of `getQueryApplicationClass()` are in `src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php` and `src/applications/policy/__tests__/PhabricatorPolicyTestCase.php`.
See T15642
Test Plan:
Patch changes only one existing code place, thus check if related pages still work as expected:
* Go to http://phorge.localhost/feed/
* Go to http://phorge.localhost/feed/transactions/
* On http://phorge.localhost/feed/transactions/ , click `Edit Query` and set `Object Types` to `Application` etc.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15642
Differential Revision: https://we.phorge.it/D25500
Summary:
Update the Application Detail view and List View to show a unified set of Badges (Deprecated, etc.), show PHIDs and Monograms on the Application Detail view, allow Applications to register Monograms.
Example of the page /applications/view/PhabricatorDiffusionApplication/:
{F393393}
T15568
Test Plan:
1. Visit /applications/ and see Deprecated badges etc.
2. Visit various Configure buttons from that list and see Monograms, Badges, PHIDs etc.
3. Enjoy screenshots in the comments of this Diff
Reviewers: O1 Blessed Committers, valerio.bozzolan, avivey, speck
Reviewed By: O1 Blessed Committers, valerio.bozzolan, avivey, speck
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Cigaryno
Differential Revision: https://we.phorge.it/D25362
Summary:
In some cases, older Herald transcripts which got garbage collected display an exception.
For unknown reasons, calling `getObjectTranscript()` on the HeraldTranscript object `$xscript` can return a bool with the value `false`.
Afterwards, calling `getAppliedTransactionPHIDs()` on that bool throws an exception.
Thus as a workaround, return an empty array instead in this case, like the code already does when $xscript->getObjectTranscript()->getAppliedTransactionPHIDs() === null.
Note that it remains unclear whether/when the bool may have the value `true` hence this situation is not covered by this code change (the code would still throw an exception in this situation).
Closes T15343
Test Plan:
1. Change system time to one year back (or optionally, use `./phorge/bin/garbage set-policy --collector herald.transcripts --days 1`, drop steps 9 and 10, and wait a day before performing step 11)
2. As an admin, go to http://phorge.localhost/herald/create/
3. Select "Maniphest Tasks"
4. On http://phorge.localhost/herald/create/?adapter=HeraldManiphestTaskAdapter , select "Global Rule"
5. Under "Conditions", select `any of` and set the three conditions `Description | contains | Internet Archive`, `Description | contains | archive.org`, `Description | contains | Wayback Machine`.
6. Under "Action", select `Add projects | someProject`
7. Select "Save Rule"
8. Trigger the rule by creating a new task.
9. Change system time back to today
10. On the shell, run `./phorge/bin/phd restart`
11. Visit created task and select "View Herald Transcript"
12. Transcript page displays "Old Transcript - Details of this transcript have been garbage collected." and shows no exception anymore
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15343
Differential Revision: https://we.phorge.it/D25246
Summary:
`preg_match()` does not accept passing null as the `$subject` string parameter in PHP 8.1.
Thus first check that `$subject !== null`.
```
EXCEPTION: (RuntimeException) preg_match(): Passing null to parameter #2 ($subject) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=0e32dbc1ac8f), phorge(head=diffusionRepoPage, ref.master=5405134fa5db, ref.diffusionRepoPage=dbe5e3a68c41)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/error/PhutilErrorHandler.php:261]
#1 <#2> preg_match(string, NULL) called at [<phorge>/src/applications/herald/typeahead/HeraldRuleDatasource.php:25]
```
Closes T15422
Test Plan: Applied this change; afterwards on the "Diffusion 🡒 Push Logs 🡒 Advanced Search" page at `/diffusion/pushlog/?repositories=PHID-REPO-someRepositoryString`, clicking the search icon for the "Blocked By" field correctly renders.the "Browse Herald Rules" overlay dialog, listing available Herald rules.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15422
Differential Revision: https://we.phorge.it/D25248
Summary:
If you want to create a new Herald Rule, sometime,
like for new "Object" Herald Rules, the Back button just
sends you to the very same page you already were.
The risk is that an employee could receive a specific
instruction, such as "Hey Alfreda, Go back, now"
and at this point Alfreda goes into an incredible loop
continuing to click the Back button until the duration
of the universe (since Phorge is so stable that it's able
to handle that "Back spike" forever - really).
https://we.phorge.it/T15184
Note that this also tries to avoid to change the base URI
to just go Back. For example, before this change, the Back
button was trying to send from /create/ to /new/, but
apparently they are just aliases.
Closes T15184
Test Plan:
- Visit the page /herald/create/?adapter=commit
- Click on "Object"
- Click on "Back"
- Verify that you only went back one screen, and not just forward in time
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15184
Differential Revision: https://we.phorge.it/D25087
Summary:
Note that PHP 8.2 implemented this deprecation:
Using ${var} in strings is deprecated, use {$var} instead
This change fixes some known cases.
Ref T15196
Test Plan: - I checked with my big eyes that everything is as it should be
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15196
Differential Revision: https://we.phorge.it/D25098
Summary: Ref T13682. Many subclasses of "CursorPagedPolicyAwareQuery" have the same implementation of "loadPage()", and this is a sensible default behavior.
Test Plan: Examined changes to verify that all removed methods have the same behavior.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21838
Summary: Ref T13588. This field may be "null" (and is probably never the empty string, but that's a more ambitious fix).
Test Plan: Ran unit tests, got a pass.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21752
Summary: Ref T13586. In the footsteps of D21563, make Herald rule results more formal and structured to support meaningful exception reporting.
Test Plan:
Ran various Herald rules and viewed transcripts, including rules with recursive dependencies and condition exceptions.
{F8447894}
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21565
Summary: Ref T13586. Lift the behavioral core of "HeraldConditionResult" into a new abstract base "HeraldTranscriptResult", with the intent to introduce a "HeraldRuleResult".
Test Plan:
- Ran Herald rules, reviewed transcripts.
- This change should have no behavioral effect.
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21564
Summary:
Ref T13586. Currently, Herald condition logs encode "pass" or "fail" robustly, "forbidden" through a sort of awkward side channel, and can not properly encode "invalid" or "exception" outcomes.
Structure the condition log so results are represented unambiguously and all possible outcomes (pass, fail, forbidden, invalid, exception) are clearly encoded.
Test Plan:
{F8446102}
{F8446103}
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21563
Summary: Ref T13586. The Herald transcript page has become more and more complicated over time, and recently added "Transactions" and "Profiler" sections. Split these across separate navigation tabs to limit the maximum complexity of any single view and make it easier to navigate to particular sections, like the profiler section.
Test Plan: Viewed various transcripts, saw nice digestible sections.
Maniphest Tasks: T13586
Differential Revision: https://secure.phabricator.com/D21493
Summary: Ref T13583. To improve support for making it harder to improperly mix data retention policies, allow Herald to act on comment content.
Test Plan:
- Wrote comment content Herald rules in Maniphest and Differential.
- Submitted non-matching comments (no action) and matching comments (Herald action).
- In Differential, triggered rules by submitting non-matching main content and a matching inline comment.
Maniphest Tasks: T13583
Differential Revision: https://secure.phabricator.com/D21479
Summary:
See PHI1794, which reports an issue where a large number of queued webhook calls led to connection exhaustion. To make this easier to reproduce and test, add "--count" and "--background" flags to "bin/webhook call".
This primarily supports "bin/webook call ... --background --count 10000" to quickly fill the queue with a bunch of calls.
Test Plan: Ran `bin/webhook call` in foreground and background modes, with and without counts. Saw appropriate console and queue behavior.
Differential Revision: https://secure.phabricator.com/D21368
Summary: Ref T13480. Creating a rule in Herald currently uses the older radio-button flow. Update it to the "clickable menu" flow to simplify it a little bit.
Test Plan: Created new personal, object, and global rules. Hit the object rule error conditions.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20956
Summary:
Ref T13480. Currently, some Herald field types are rendered in an unfriendly way on transcripts. Particularly, PHID lists are rendered as raw PHIDs.
Improve this by delegating rendering to Value objects and letting "PHID List" value objects render more sensible handle lists. Also improve "bool" fields a bit and make more fields render an explicit "None" / empty value rather than just rendering nothing.
Test Plan: Viewed various transcripts, including transcripts covering boolean values, the "Always" condition, large blocks of text, and PHID lists.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20951
Summary:
Ref T13480. Currently, when Herald renders a transcript, it puts display labels into array keys. This is a bad pattern for several reasons, notably that the values must be scalar (so you can't add icons or other markup later) and the values must be unique (which is easily violated because many values are translated).
Instead, keep values as list items.
Test Plan: Viewed Herald transcripts, saw no (meaningful) change in rendering output.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20949
Summary: Ref T13480. When Herald renders rules, it partly uses a very old handle pre-loading mechanism where PHIDs are extracted and loaded upfront. This was obsoleted a long time ago and was pretty shaky even when it worked. Get rid of it to simplify the code a little.
Test Plan: Viewed Herald rules rendered into static text with PHID list actions, saw handles. Grepped for all affected methods.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20948
Summary: Ref T13480. These fields don't serve a specific strong use case, but are broadly reasonable capabilities after "state" vs "change" actions were relaxed by T13283.
Test Plan: Wrote rules using the new fields, added and removed projects (and did neither) to fire them / not fire them. Inspected the transcripts to see the project PHIDs making it to the field values.
Maniphest Tasks: T13480
Differential Revision: https://secure.phabricator.com/D20946
Summary: Fixes T13445. Make the meaning of this condition more clear, since the current wording is ambiguous between "any of" and "all of".
Test Plan: Edited a Herald rule with a PHID list field ("Project tags"), saw text label say "include none of".
Maniphest Tasks: T13445
Differential Revision: https://secure.phabricator.com/D20889
Summary:
Fixes T7961. Currently, we present Herald users with actions like "Require legalpad signatures" and "Run build plans" even if Legalpad and Harbormaster are not installed.
Instead, allow fields and actions to be made "unavailable", which means that we won't present them as options when adding to new or existing rules.
If you edit a rule which already uses one of these fields or actions, it isn't affected.
Test Plan:
- Created a rule with a legalpad action, uninstalled legalpad, edited the rule. Action remained untouched.
- Created a new rule, wasn't offered the legalpad action.
- Reinstalled the application, saw the action again.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T7961
Differential Revision: https://secure.phabricator.com/D20808
Summary:
Fixes T9136.
- Fix a bug where the name is rendered improperly.
- Put disabled rules at the bottom.
- Always show the rule monogram so you can distingiush between rules with the same name.
Test Plan: {F6849915}
Maniphest Tasks: T9136
Differential Revision: https://secure.phabricator.com/D20798
Summary:
Fixes T8952. These feed stories are not interesting and tend to be generated as collateral damage when a non-story update is made to an old task and someone has a "subscribe me" Herald rule.
Also clean up some of the Herald field/condition indexing behavior slightly.
Test Plan: Wrote a "Subscribe X" herald rule, made a trivial update to a task. Before: low-value feed story; after: no feed story.
Maniphest Tasks: T8952
Differential Revision: https://secure.phabricator.com/D20797
Summary:
Fixes T13408. Currently, when a package (or other object) appears in a field (rather than an action), it is not indexed.
Instead: index fields too, not just actions.
Test Plan:
- Wrote a rule like "[ Affected packages include ] ...".
- Updated the search index.
- Saw rule appear on "Affected By Herald Rules" on the package detail page.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13408
Differential Revision: https://secure.phabricator.com/D20795
Summary: Ref T13319. Ref PHI1302. Migrate `PhabricatorEditEngineConfigurationTransaction` to modular transactions and add some additional transaction rendering to make these edits less opaque.
Test Plan: Hit all the form edit controllers, viewed resulting transaction timeline.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T13319
Differential Revision: https://secure.phabricator.com/D20595
Summary:
Ref T13298. Add a simple profiler as a starting point to catch any egregiously expensive rules or conditions.
This doesn't profile rule actions, so if "Add subscriber" (or whatever) is outrageously expensive it won't show up on the profile. Right now, actions get evaluated inside the Adapter so they're hard to profile. A future change could likely dig them out without too much trouble. I generally expect actions to be less expensive than conditions.
This also can't pin down a //specific// condition being expensive, but if you see that `H123` takes 20s to evaluate you can probably guess that the giant complicated regex is the expensive part.
Test Plan: {F6473407}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13298
Differential Revision: https://secure.phabricator.com/D20566
Summary:
Depends on D20566. Ref T13298. See PHI1280. Currently, there's no clean way to disable problematic personal rules. This comes up occasionally and sometimes isn't really the best approach to solving a problem, but is a generally reasonable capability to provide.
Allow Herald rules (including personal rules) to be disabled/enabled via `bin/herald rule ... --disable/--enable`.
Test Plan: Used the CLI to disable and enable a personal rule.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jmeador
Maniphest Tasks: T13298
Differential Revision: https://secure.phabricator.com/D20567
Summary:
Depends on D20546. Ref T13283. Currently, if you do something (transactions "A", "B") and Herald does some things in response (transaction "C"), Herald acts only on the things you did ("A", "B") since the thing it did ("C") didn't exist yet, until it ran.
However, if you use the test console to test rules against the object we'll pick up all three transactions since they're all part of the same group. This isn't ideal.
To fix this, skip transactions which Herald applied, since it obviously didn't consider them when it was evaluating.
Test Plan:
- Created a revision, in the presence of a Herald rule that adds reviewers.
- Then, ran the revision through the test console.
- Before: saw the "Herald added reviewers: ..." transaction in the transaction group Herald evaluated.
- After: saw only authentic human transactions.
{F6464064}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20547
Summary:
Ref T13283. See PHI1202. See D20519. When we apply a group of transactions, label all of them with the same "group ID".
This allows other things, notably Herald, to figure out which transactions applied together in a faithful way rather than by guessing, even though the guess was probably pretty good most of the time.
Also expose this to `transaction.search` in case callers want to do something similar. They get a list of transaction IDs from webhooks already anyway, but some callers use `transaction.search` outside of webhooks and this information may be useful.
Test Plan:
- Ran Herald Test Console, saw faithful selection of recent transactions.
- Changed hard limit from 1000 to 1, saw exception. Users should be very hard-pressed to hit this normally (they'd have to add 990-ish custom fields, then edit every field at once, I think) so I'm just fataling rather than processing some subset of the transaction set.
- Called `transaction.search`, saw group ID information available.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20524
Summary: Depends on D20519. Ref T13283. See PHI1202. Add a new rule which triggers when the current/most-recent transaction group includes a "content" or "publish" transaction, which means the published document content has changed.
Test Plan:
- Wrote a Herald rule using this field.
- Created a document (rule matched).
- Edited a document (rule matched).
- Edited a document, saving as a draft (no match).
- Edited a draft, updating it (no match).
- Published a draft docuemnt (rule matched).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20520
Summary:
Depends on D20518. Ref T13283. When you use the "Test Console" in Herald to test rules, pass the most recent group of transactions to the Adapter.
This will make it easier to test rules that depend on edit state, since you can make the type of edit you're trying to test and then use the Test Console to actually test if it matches in the way you expect.
The transactions we select may not be exactly the group of transactions that most recently applied. For example, if you make edits A, B, and C in rapid succession and then run the Test Console on the object, it may select "A + B + C" as a transaction group. But we'll show you what we selected and this is basically sane/reasonable and should be fine.
(Eventually, we could include a separate "transaction group ID" on transactions if we want to get this selection to match exactly.)
Test Plan: Ran the Test Console on various objects, saw sensible transaction lists in the transcripts.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20519
Summary:
Ref T13283. Since D14575, we already pass applied transactions to Herald, but they exist only as a backwards compatibility layer and have no upstream callsites.
Save the applied transaction PHIDs as part of the object transcript, and show them in the UI.
Test Plan:
- Viewed a modern transcript, saw a list of transactions.
- Viewed an older transcript, saw nothing (since there were no transactions in the transcript).
{F6456431}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13283
Differential Revision: https://secure.phabricator.com/D20518
Summary:
Depends on D20397. Ref T13272. Similar to the recent "where are Herald rules used" stuff, show which menus Dashboards are installed in.
This is mostly straightforward, except that I pulled some of the Herald logic into a parent class so it could be shared.
Test Plan: {F6369164}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13272
Differential Revision: https://secure.phabricator.com/D20398
Summary:
Depends on D20259. Now that we can index Herald rules to affected objects, show callers on the "Webhooks" UI.
A few other rule types could get indexes too ("Sign Legalpad Documents", "Add Reviewers", "Add Subscribers"), but I think they're less likely to be useful since those triggers are usually more obvious (the transaction timeline makes it clearer what happened/why). We could revisit this in the future now that it's a possibility.
Test Plan: {F6260106}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D20260
Summary:
Ref T13258. Provide an easy way to find rules which trigger a particular build plan from the build plan page.
The implementation here ends up a little messy: we can't just search for `actionType = 'build' AND targetPHID = '<build plan PHID>'` since the field is a blob of JSON.
Instead, make rules indexable and write a "build plan is affected by rule actions" edge when indexing rules, then search on that edge.
For now, only "Run Build Plan: ..." rules actually write this edge, since I think (?) that it doesn't really have meaningful values for other edge types today. Maybe "Call Webhooks", and you could get a link from a hook to rules that trigger it? Reasonable to do in the future.
Things end up a little bit rough overall, but I think this panel is pretty useful to add to the Build Plan page.
This index needs to be rebuilt with `bin/search index --type HeraldRule`. I'll call this out in the changelog but I'm not planning to explicitly migrate or add an activity, since this is only really important for larger installs and they probably (?) read the changelog. As rules are edited over time, this will converge to the right behavior even if you don't rebuild the index.
Test Plan: {F6260095}
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13258
Differential Revision: https://secure.phabricator.com/D20259
Summary:
Ref T13249. See PHI1115. I initially wanted to make `bin/policy unlock --owner <user> H123` work to transfer ownership of a Herald rule, although I'm no longer really sure this makes much sense.
In any case, this makes things a little better and more modern.
I removed the storage table for rule comments. Adding comments to Herald rules doesn't work and probably doesn't make much sense.
Test Plan: Created and edited Herald rules, grepped for all the transaction type constants.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13249
Differential Revision: https://secure.phabricator.com/D20258
Summary:
See PHI1096. Depends on D20213. An install is reporting a hard-to-reproduce issue where a non-transaction gets queued by Herald somehow. This might be in third-party code.
Sprinkle the relevant parts of the code with `final` and type checking to try to catch the problem before it causes a fatal we can't pull a stack trace out of.
Test Plan: Poked around locally (e.g., edited revisions to cause Herald to trigger), but hard to know if this will do what it's supposed to or not without deploying and seeing if it catches anything.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20214
Summary: Ref T13250. See D20149. In a number of cases, we use `setQueryParams()` immediately after URI construction. To simplify this slightly, let the constructor take parameters, similar to `HTTPSFuture`.
Test Plan: See inlines.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20151
Summary: See PHI1068. We currently show "HTTP Error - 200", which is misleading. Instead, label these results as "HTTP Status Code".
Test Plan: {F6206016}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D20148
Summary:
Depends on D20115. See <https://discourse.phabricator-community.org/t/transaction-search-endpoint-does-not-work-on-differential-diffs/2369/>.
Currently, `getApplicationTransactionCommentObject()` throws by default. Subclasses must override it to `return null` to indicate that they don't support comments.
This is silly, and leads to a bunch of code that does a `try / catch` around it, and at least some code (here, `transaction.search`) which doesn't `try / catch` and gets the wrong behavior as a result.
Just make it `return null` by default, meaning "no support for comments". Then remove the `try / catch` stuff and all the `return null` implementations.
Test Plan:
- Grepped for `getApplicationTransactionCommentObject()`, fixed each callsite / definition.
- Called `transaction.search` on a diff with transactions (i.e., not a sourced-from-commit diff).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: jbrownEP
Differential Revision: https://secure.phabricator.com/D20121
Summary:
Depends on D19928. See <https://discourse.phabricator-community.org/t/firehose-webhook-not-working-with-self-hosted-requestbin-instance/2240/>.
Currently, we report "hook" and "silent", which are raw internal codes.
Instead, report human-readable labels so the user gets a better hint about what's going on ("In Silent Mode").
Also, render a "hey, you're in silent mode so none of this will work" reminder banner in this UI.
Test Plan:
{F6074421}
Note:
- New warning banner.
- Table has more human-readable text ("In Silent Mode").
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19929
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
Summary: Ref T13216. See D19666. It's currently tricky to profile Herald test runs since you have to submit a form and repeating them is a bit of a mess. Provide a simple CLI wrapper so we can use `--xprofile`. This is also maybe nice-to-have if we're ever debugging anything here.
Test Plan: Ran `bin/herald test --object ... --type ...` and got a sensible looking transcript in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13216
Differential Revision: https://secure.phabricator.com/D19806
