Also fix the typo in the function name

Thanks. Your edit effectively reflects what already happens, but interestingly, it still may crash 🤔

@valerio.bozzolan: Y U so sirious??? :D

A bit more than Normal, since it reflects on a database with orphan elements that creates "ghosts in the UI".

Wow. With my proposal is not funny anymore. OK I will refrain myself from contributing here. But I personally like the new proposal from Andrè and @pppery. Personally approving.

I understand the points. Third proposal, starting with useful tips (good idea), and shorter:

I like the change in language here, but maybe a total rewrite of the text is better. My take:

When setting a new password. please keep it safe, using a trustworthy password manager and a randomly generated password.
We thank you, your administrator thanks you, and you'll thank yourself in the future.

In the unlikely event you that absolutely don't intend to keep it safe at all, you might choose instead, to:

• Write it down on a sticky note, put it on your monitor, and hope nobody else uses it on %s.
• Use the same password you've already used, especially for your e-mail and bank account.
• Use easy-to-remember, easy-to-guess passwords, like "12345abcde".

Modern security advice considers these practices "a bad idea".
If you're already doing any of these, here or elsewhere, you might want to consider the account compromised.

Thanks! Confirming that the link on https://we.phorge.it/book/dev/class/PhutilDaemon/ is a 404, and that the file was deleted in https://secure.phabricator.com/rPHU720c8116845bb9dc19334170e6c0702aa0210c78

Fix a double space after testing

Actually do that, since I screwed up before

Update to Andre's suggestion

Tested this with nearly all DataSources mentioned above before and after applying the patch in both a logged-in and a private browser window. Behaves as expected and no errors in the logs.

No results.

Is anything extending HeraldEngine trying direct access to that?

Please add some negative tests - things that could trigger this thing, but shouldn't.

The linter now screams nonsense things because of my little new unit test, taken from the proposed tar.gz file. Probably better than nothing. Tested A/B and it makes sense. Also the addition makes sense to me, compared to the other ones.

add unit test

• AlmanacDeviceSearchEngine: add comma, but maybe keeping 'networks' in plural since the button is See Networks
• AlmanacServiceSearchEngine: -of your devices +provided by your devices

Note that the responsible users' projects and packages should absolutely still be included in the reviewers query join (line 487 right-side). This is why $this->responsibles remains unmodified.

In D25676#18547, @aklapper wrote:

@jmeador Hi, what would be the Test Plan to get a "resulting query"? Going to /differential/query/advanced/, selecting the magnifier button for the Responsible Users field, and checking that after applying the patch only user accounts are listed, and no more projects or packages, I assume?

@jmeador Hi, what would be the Test Plan to get a "resulting query"? Going to /differential/query/advanced/, selecting the magnifier button for the Responsible Users field, and checking that after applying the patch only user accounts are listed, and no more projects or packages, I assume?

All in all, this looks ready to merge after removing two commas and fixing one BE spelling. I tested this locally and everything worked as expected.

I'd also prefer American English spelling for the sake of consistency, means: using catalog instead of catalogue.
cf https://we.phorge.it/source/arcanist/browse/master/src/workflow/ArcanistWorkflow.php$840 :P

I confirm that all other 21 definitions of function getNewUserBody() are in *Engine.php files/classes, except the one fixed/moved by this very patch.

Abiding by the law of triviality, after nine meetings the Working Group that I set up for this task came up with this proposal:

I think the EditEngine is what's used to create the actions form, and it has some sense of the object's status (see for example the available actions on Revisions - these change based on the revision's state).
Maybe it can get an additional "field" for this warning, and display it based on task status.

I remain unhappy with my code in D25546:

• Phorge lacks a method "give me all engines for this application".
• PhabricatorApplication offers nothing related to engines.
• PhabricatorEditEngine::getApplication() does what I want exactly the other way round: it returns the application for a given engine.
• PhabricatorEditEngine::getAllEditEngines() is confusingly named. It only returns EditEngine keys like calendar.export or maniphest.task but not actual engines or engine classes. ($engines = id(new PhabricatorEditEngineQuery())->setViewer($this->getViewer())->execute(); returns the actual engines, as already used by this code.)
• I cannot find some mapping between EditEngine keys (like maniphest.task) and either PhabricatorPHIDTypes (like ManiphestTaskPHIDType) or their TypeConstants (like TASK) either.
• getEngineClassName() exists but only in a SearchEngine/SearchQuery context, not in a EditEngine content (and I cannot find its constructor)

I remain unhappy with this code.

• Phorge lacks a method "give me all engines for this application".
• PhabricatorApplication offers nothing related to engines.
• PhabricatorEditEngine::getApplication() does what I want exactly the other way round: it returns the application for a given engine.
• PhabricatorEditEngine::getAllEditEngines() is confusingly named. It only returns EditEngine keys like calendar.export or maniphest.task but not actual engines or engine classes. ($engines = id(new PhabricatorEditEngineQuery())->setViewer($this->getViewer())->execute(); returns the actual engines, as already used by this code.)
• I cannot find some mapping between EditEngine keys (like maniphest.task) and either PhabricatorPHIDTypes (like ManiphestTaskPHIDType) or their TypeConstants (like TASK) either.
• getEngineClassName() exists but only in a SearchEngine/SearchQuery context, not in a EditEngine content (and I cannot find its constructor)

Use phid_get_type instead of substr as it does the same job

Also adding some on-platform documentation would be nice.

Screenshot taken at 702 x 495

Okay I'll just re-arrange the chairs on the ship deck a bit then, double-swear!

I guess I should abandon this patch?

Nah, appreciated if you think that there is a better / more correct way to fix this which also covers a bigger underlying issue that you found. :)
I guess I should abandon this patch?

No, limit is working fine as it is.

Sorry again for the confusion, I don't want to create extra work. I will create another patch as clarification. Unfortunate legacy situation.

Yup. I noticed that probably the method getNewUserBody() should be just moved from PhabricatorDashboardPanelListController to PhabricatorDashboardPanelSearchEngine. Additional details in T15844.

In D25649#18446, @valerio.bozzolan wrote:

Just try to move the method in PhabricatorDashboardPanelSearchEngine as-is