Problem/Motivation

So when defining a local action, you may specify a class to dynamically set some properties. A common use case is to slap on a 'destination' query parameter so people are redirected to the page the action was on after performing the action itself.

The menu_ui module already provides a class that does exactly that: \Drupal\menu_ui\Plugin\Menu\LocalAction\MenuLinkAdd. Why don't we generalize this class to core to something like: Drupal\Core\Menu\LocalActionWithDestination?

That way, all modules can use it without having to rely on menu_ui being enabled or having to duplicate the code.

Steps to reproduce

Not applicable.

Proposed resolution

Remaining tasks

Confirm the changes referred to at https://git.drupalcode.org/project/drupal/-/merge_requests/10366#note_41...

User interface changes

None.

Introduced terminology

None.

API changes

None.

Data model changes

None.

Release notes snippet

None.

CommentFileSizeAuthor
#32 interdiff_29-32.txt760 bytesravi.shankar
#32 2762131-32.patch8.45 KBravi.shankar
#29 interdiff_26-29.txt466 bytesvsujeetkumar
#29 drupal-2762131-29.patch8.48 KBvsujeetkumar
#27 interdiff-2762131-25-26.txt1.31 KBmohit_aghera
#27 drupal-2762131-26.patch8.48 KBmohit_aghera
#25 interdiff-2762131-14-25.txt3.78 KBmohit_aghera
#25 drupal-2762131-25.patch8.42 KBmohit_aghera
#23 drupal-2762131-23.patch3.41 KBsolimanharkas
#23 interdiff_14-23.txt2.81 KBsolimanharkas
#14 drupal-2762131-14.patch6.17 KBkristiaanvandeneynde
#2 drupal-2762131-2.patch6.18 KBkristiaanvandeneynde

Issue fork drupal-2762131

Command icon Show commands

Start 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:

About issue forks

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new drupal-2762131-2.patch6.18 KB

Let's see what testbot thinks of it. Should be a pretty straightforward patch and has BC.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented 
+++ b/core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
@@ -2,65 +2,13 @@
+ * ¶

Hmm, looks like PhpStorm missed this whitespace.

bojanz’s picture

bojanz commented

The patch and the idea make sense. Committable in its current shape.

The old class didn't have a test, does a new one need it?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Thanks for the review, bojanz!

As I mentioned on IRC, what's happening here is:

  • The service 'redirect.destination' is being used; should have test coverage.
  • A local action is being altered through a class specified in its YAML definition; should have test coverage.
  • A link is being generated with a destination parameter; should have test coverage.

So all of the functionality is already being tested on a more granular/topical scale. I would love to write a test for this, although I fear it would simply replicate that which is already being tested.

dawehner’s picture

dawehner
Primary language German
commented 
+++ b/core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
@@ -2,65 +2,13 @@
+ * ¶

Nitpick: whitespace :P

Well some form of kernel test or so would be great I guess. Something which ensures that the test fails in case this class is broken in some way.

dawehner’s picture

dawehner
Primary language German
commented

Oh yeah in general I totally agree that this class makes 100% sense. Nice addition

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

psf_’s picture

psf_ commented
Status: Needs review » Reviewed & tested by the community

Work fine in D8.6.1

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
StatusFileSize
new drupal-2762131-14.patch6.17 KB

Rerolling and seeing what testbot thinks of it nowadays

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

I can imagine it needs the deprecation handling mentioned here https://www.drupal.org/core/deprecation#how-class

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: drupal-2762131-14.patch, failed testing. View results

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs work » Reviewed & tested by the community

Testbot hiccup. Setting back to RTBC but still on the fence re deprecation handling.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests 
  1. +++ b/core/lib/Drupal/Core/Menu/LocalActionWithDestination.php
@@ -0,0 +1,64 @@
+    $options['query']['destination'] = $this->redirectDestination->get();

    should we be using \Drupal\Core\Routing\RedirectDestination::getAsArray here? 

  2. +++ b/core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
@@ -2,65 +2,13 @@
+ * @deprecated in Drupal 8.6.1 and will be removed before Drupal 9.0.0. Use
+ *   \Drupal\Core\Menu\LocalActionWithDestination instead.

    we need a trigger_error here as per our policy https://www.drupal.org/core/deprecation#how-class

    we also need a deprecation test to ensure we have the error

Also #6 asked for a test, but I don't see them yet.

This is a great DX feature, thanks for working on it!

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

askibinski’s picture

askibinski commented
Related issues: +#1344902: Provide an optional destination parameter for local actions

Related: #1344902: Provide an optional destination parameter for local actions

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

solimanharkas’s picture

solimanharkas commented
StatusFileSize
new interdiff_14-23.txt2.81 KB
new drupal-2762131-23.patch3.41 KB

...attached wrong patch, will upload the right one shortly! sorry!

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

mohit_aghera’s picture

mohit_aghera commented
StatusFileSize
new drupal-2762131-25.patch8.42 KB
new interdiff-2762131-14-25.txt3.78 KB

- Fix the comments mentioned in #18
- Add test cases.

I noticed that the deprecation method for plugin is somewhat different as per the doc. I've updated it.

mohit_aghera’s picture

mohit_aghera commented
Status: Needs work » Needs review
mohit_aghera’s picture

mohit_aghera commented
StatusFileSize
new drupal-2762131-26.patch8.48 KB
new interdiff-2762131-25-26.txt1.31 KB

Fixing phpcs and other deprecation coding standard issues.

Status: Needs review » Needs work

The last submitted patch, 27: drupal-2762131-26.patch, failed testing. View results

vsujeetkumar’s picture

vsujeetkumar commented
Status: Needs work » Needs review
StatusFileSize
new drupal-2762131-29.patch8.48 KB
new interdiff_26-29.txt466 bytes

Fixing fail test.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

idebr’s picture

idebr commented
Status: Needs review » Needs work

Needs work per #18.1

ravi.shankar’s picture

ravi.shankar commented
Status: Needs work » Needs review
StatusFileSize
new 2762131-32.patch8.45 KB
new interdiff_29-32.txt760 bytes

Addressed comment #31.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: -Needs tests +Needs Review Queue Initiative, +Needs change record

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Maybe it's just me but if changing the class name couldn't that break existing sites?
If so we would need a different approach, maybe we deprecate the old

If not still think this could use a change record.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

tstoeckler made their first commit to this issue’s fork.

tstoeckler opened merge request !10366

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review
Issue tags: -Needs change record

If so we would need a different approach, maybe we deprecate the old

I don't understand, that's exactly what the patch does.

Added a draft change notice and converted to merge request.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented

There was an issue in #32 in how getAsArray() was used, that is fixed now and, thus, the tests are green again. Good to go from my point of view.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Needs work

Don't agree with #18.1, but other than that changes look good minus some tiny things. Left notes. Thanks for converting this @tstoeckler!

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs work » Needs review

Thanks for the review, fixed all the things now (hopefully).

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

LGTM now. As I said before, I don't think we need \Drupal\Core\Routing\RedirectDestination::getAsArray() here because AFAIAC it doesn't make sense in this context.

I saw one seemingly random test failure, so running that one again. Can RTBC if that one goes green.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Status: Needs review » Reviewed & tested by the community

All green now.

Saving credit for Tobias, might remove some credit for broken patches but asking internally first.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
  • Hid all patches as we switched to MR
  • Saved credit for the code contributions, leaving credit for reviews to the core committer
  • Removed credit for comments that only uploaded broken patches
  • On the fence about #29 as it's a tiny change but kept credit for now as it was to some extent impactful
  • Will also update CR to be a bit more informative
quietone’s picture

quietone commented
Issue summary: View changes

Read the IS, comments, the MR and the Change record. For the MR, a change was requested by larowlan, which has not happened. It is explained in the comments in the MR but that should have another check, it is at https://git.drupalcode.org/project/drupal/-/merge_requests/10366#note_41.... The other was the change record which I edited to focus on the impact and what sites need to do. I put the 'what is changed' at the top, followed by the action sites may need to take. and removed bits explaining how the deprecation is done.

I also updated credit.

alexpott made their first commit to this issue’s fork.

oily’s picture

oily commented
Issue summary: View changes

Filled out the issue summary template.

oily’s picture

oily commented

Added one code comment/ recommendation to the MR.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Added to the discussion regarding the wording of a test case, but don't think that warrants removing RTBC. A committer an choose between the current docs and the ones suggested on commit.

  • alexpott committed b2205aff on 11.x 
    Issue #2762131 by tstoeckler, mohit_aghera, kristiaanvandeneynde,...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed b2205af and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.