ViewsUI::getStandardButtons() has some magic to find cancel submit handler. It looks for functions with the same name as the form ID, with the suffix _cancel.
The code looks like this, and has existed since at least views 6.x-2.x in contrib.
$cancel_submit = function_exists($form_id . '_cancel') ? $form_id . '_cancel' : [$this, 'standardCancel'];
But this is a relic to a time when forms exists in procedural functions in the global namespace. Forms now belong in their own classes, with submit handlers bundled in to the class. There are no _cancel() functions left in core that are associated with forms.
There is a possibility existing contrib or custom modules are relying on this behaviour, but if these functions exist in .module files, they will stop working once .module files are deprecated and removed. By deprecating now these can be updated to manually specify the submit handlers in the form class.
$ grep -r "function .*_cancel(" core
core/modules/user/user.api.php:function hook_user_cancel($edit, UserInterface $account, $method): void {
core/modules/user/user.module:function user_cancel($edit, $uid, $method): void {
core/modules/user/user.module:function _user_cancel($edit, $account, $method): void {Refactor $cancel_submit to throw a deprecation if the function_exists call finds something.
| Comment | File | Size | Author |
|---|
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !12734
Comments
Comment #3
mstrelan commentedDid the deprecation, needs a change record. Could possibly do a deprecation test but it would involve adding a test module with a form that calls
$view->getStandardButtonssimilar to\Drupal\views_ui\Form\Ajax\ReorderDisplaysand also declare a function with the magic naming convention.Comment #4
volegerLink related issue https://www.drupal.org/project/drupal/issues/3043725
Comment #5
mstrelan commentedI don't think that issue is related. The grep in the summary is showing that there are no _cancel functions related to views forms. The ones remaining are for the hook, which is what the related issue is about. I can see how that has led to confusion though.
Comment #6
nicxvan commentedThis needs a CR and the deprecation needs to be updated with the correct CR.
Deprecations don't need a test, the rest of the MR does look good.
I removed the unrelated issue.
Comment #7
mstrelan commentedAgree we don't normally need a test for a deprecation, unless it's a weird implementation. I think what I really meant that we don't actually have any evidence that the behaviour we're deprecating even works in the first place, so we could test for that as a deprecation test. But I don't know if it's worth it.
Comment #8
catchIt feels very unlikely that anyone is relying on this, and fairly unlikely that it even works at all, so for me fine to skip deprecation test coverage here.
Comment #9
nicxvan commentedGreat thanks! I created a CR and added a suggestion to change the deprecation to use the CR.
It could use a pass.
Comment #10
mstrelan commentedApplied the suggestion from the MR and updated the CR to make it specific to Views UI and with a more realistic example.
Comment #11
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.
Comment #12
mstrelan commentedComment #13
nicxvan commentedHuh it seemsmy comment was eaten but you got my suggestions anyway.
CR is far better now thanks!
Deprecation is correct now too.
Good to go.
Comment #14
xjmNice find. This is an excellent cleanup, but we should probably have subsystem maintainer signoff to ensure we're not leaving behind any trailing related bits etc. I'm technically only a maintainer for Views, not Views UI, so bumping back to NR. I'll also ping Lendude.
Comment #15
xjmWe might want to target Drupal 13 for this deprecation. I tried to search contrib to see if this was widely used still, but ran into problems with the fuzziness (it refusing to do an exact search for
_cancel().Comment #16
lendudeCan't think of (or find) anything either that could possibly rely on this. So great clean up as far as I can tell.
Comment #17
mstrelan commentedSo is this RTBC or need to be updated for D13? It seems incredibly niche.
Comment #18
xjmI agree that it does seem very niche, but since it is a very dynamic public API that could be used in a lot of ways and I couldn't get a good sense of how many might still exist from the contrib search, I think I'd still deprecate it for Drupal 13 to be safe.
.modulefiles themselves aren't going to be removed before then, either, so it shouldn't be problematic to keep them around the extra two years. Thanks @mstrelan!Comment #19
mstrelan commentedApparently this is a known issue - https://git.drupalcode.org/help/user/search/advanced_search.md#known-issues
So perhaps http://codcontrib.hank.vps-private.net/search?text=_cancel%28&filename= is a better way to search.
Bumped removed version to 13.
Comment #20
nicxvan commentedWe got sign off from the subsystem maintainer and bumped the removed version.
Good to go!
Comment #21
xjmThe main problem with the search was actually not being able to get a string literal match for the underscore, even when I dropped the parenthesis. But anyway, it's fine with a D13 deprecation and @lendude's signoff. Thanks!
I made a couple small fixes to the CR and fixed a broken link in the MR.
Comment #23
xjmCommitted to 11.x and published the change record. Thanks!
Comment #25
nicxvan commentedReparenting