Reminder to myself:

• Goal is to restrict number of edge edits within one single action for less trusted users.
• Goal is not to restrict the overall number of edges but that is what the code currently does as it relies on getMaximumSelectionSize() - it seems.

Given that folks could remove and add numerous edges in one single action (so the resulting amount of edges would be the same) I intially thought this might require a sizeof(array_diff()) on the transaction PHIDs, but I'm wrong as TaskMergeInRelationship::getMaximumSelectionSize() is only about the transaction and in total a task can have more than 5 duplicates. The willUpdateRelationships addition in https://phabricator.wikimedia.org/rPHAB4f4cae8e183d76bc758b5aaa5c966d5a607e027a might be a relevant factor in this case, plus definitely some JS kicking in (disabling the buttons in the dialog) which I have not located yet.

Summary: I still have not fully understand the relevant code.

Taking notes if someone ever needs to dig into this again:

• webroot/rsrc/js/core/behavior-object-selector.js defines the JS behavior in the overlay dialog.
• Its $maximum value per one action comes from getMaximumSelectionSize() called in PhabricatorObjectSelectorDialog.
• Its setMaximumSelectionSize() is in upstream only called in PhabricatorSearchRelationshipController which beforehand does $maximum = $relationship->getMaximumSelectionSize();.
• What confused me was "Closing as a Duplicate" allowing to select another task when a task is already closed as a duplicate: The JS dialog shows "Nothing attached", b/c merging falls into $relationship->canUndoRelationship() being false in PhabricatorSearchRelationshipController.
  • I had not realized that very aspect when I deployed rPHAB7e1314fcce2ec2bb1d19e528163b517d17e93ea6.
• The only implementation of getMaximumSelectionSize() actually returning a value in upstream is currently in src/applications/maniphest/relationship/ManiphestTaskCloseAsDuplicateRelationship.php. Per my previous bullet point this isn't taken into account.

Open interpretation question:
In my own words, PhabricatorObjectSelectorDialog::setMaximumSelectionSize() attempts to define the maximum number of objects which a user can select in the dialog. It is semantically unclear whether that's the number of objects (to create an edge each) a user "additionally" selects in the JS dialog, or the newly selected + the already existing/selected ones.
Even when changing the JS to behave like the former interpretation, the PHP part afterwards as a "sanity check" bails out on if ($maximum && (count($phids) > $maximum)) in PhabricatorSearchRelationshipController.

In any case, in webroot/rsrc/js/core/behavior-object-selector.js, if (JX.keys(phids).length < config.maximum) does not compare against the number of newly selected objects to be added as edges, but it compares against the number of already existing edges + edges to be added. Same for that PHP "sanity check".
Thus we get the annoying and unwanted behavior described in T376301: Can't edit subtasks: "Too many relationships (25, of type "task.has-subtask"). Must be less than 20.".

As this is about unwanted edits, going for the to-be-added selection as the maximum would also need to take into account removing edges. That's doable in PHP but there is no JS code to render the "Remove" button.disabled = is_disabled; in JS, but only for the "Select" buttons behavior-object-selector.js.

Given how complex "doing it right" gets regarding maintenance costs, it may make more sense to rip out all our custom code previously added in rPHAB7e1314fcce2ec2bb1d19e528163b517d17e93ea6 across all those PHP files to not annoy trusted users anymore like we do now, and introduce one single check before https://phabricator.wikimedia.org/source/phabricator/browse/wmf%252Fstable/src/applications/search/controller/PhabricatorSearchRelationshipController.php;7e1314fcce2ec2bb1d19e528163b517d17e93ea6$49-58 which introduces a single arbitrary low $maximum value for any relationship/edge setting attempts which is only applied to less trusted users, and compare it against $add_phids + $rem_phids (calculated later in that very file), and if $maximum is exceeded let less trusted users run into that non-JS "sanity check" fallback error.

aklapper opened https://gitlab.wikimedia.org/repos/phabricator/phabricator/-/merge_requests/73

Restrict number of edges to edit in one transaction for less trusted users

If you want to fix it right, maybe we should take this upstream?

Relatedly: I think we should formalize the "trusted contributors" group as that is also used upstream now.

Ideally you'd want a rate limit for this kind of thing, ie. "cannot change more than X edges within Y seconds". Some googling suggests Phabricator does rate limiting via PhabricatorSystemActionEngine::willTakeAction() so maybe that can just be called wherever Phabricator actually does the graph updates?