Provide an optional destination parameter for local actions

I agree!

Btw, here's one way to do it, for anyone who stumbles on this:

https://mushtaqtahir.com/blog/12/drupal-8-add-redirect-destination-in-ac...

Actually, the MenuLinkAdd class does this (and only this), so all that's needed in your MODULE.links.action.yml file is to define the 'class' option to this MenuLinkAdd class, like so:

custom.add_article:
  route_name: node.add
  title: 'Add article'
  route_parameters:
    node_type: 'article'
  class: \Drupal\menu_ui\Plugin\Menu\LocalAction\MenuLinkAdd
  appears_on:
    - view.articles.page_1

Looking at #6 does it exactly as required. But I think it is not right for other modules to freeload on the menu_ui module. I think that this should be moved (ultimately) to the class name of something like Drupal\Core\Menu\LocalActionAppendDestination so that any module can used this without fear of being broken in a future release.

This should not be renamed, as it will break a peoples modules which already use this. We should copy to the new path, and mark MenuLinkAdd as depreciated.

#3033960: After adding media, return the user to the media listing they started from is very much related.

The idea is to update the default Action Link plugin to support destination parameter.
In the definition of action links (yml) new parameter is introduced - add_destination: true.
If this is added, destination is appended to the action link button.

After discussion with @alexpott, it was suggested that action links should not have destination parameters by default, due to so many security issues that were related to destination itself.
We are keeping the default functionality as it was, but there is a possibility to control it in the appropriate yml file.

Original class is marked as deprecated.

Tests are added.

This makes a lot of sense to me and I think it's a nice change. I think we may need explicit tests of LocalActionDefault, but the stuff added here may suffice. I'll leave that choice up to a committer.

1. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -26,6 +27,13 @@ class LocalActionDefault extends PluginBase implements LocalActionInterface, Con
   +  private $redirectDestination;
   

   Needs to be protected.

2. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -38,10 +46,11 @@ class LocalActionDefault extends PluginBase implements LocalActionInterface, Con
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, RedirectDestinationInterface $redirect_destination) {
        parent::__construct($configuration, $plugin_id, $plugin_definition);
    
        $this->routeProvider = $route_provider;
   +    $this->redirectDestination = $redirect_destination;
   

   We'll need to default $rediect_destination to NULL and pull it out of \Drupal::service() if so. See \Drupal\config\Controller\ConfigController::__construct() for an example of this.

3. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -119,7 +129,14 @@ public function getRouteParameters(RouteMatchInterface $route_match) {
   +    if (isset($definition['add_destination']) && $definition['add_destination'] === TRUE) {
   

   I think this should probably be an option. Also, we can make this !empty($options['add_destination']), which covers both the isset() and truthy checks.

4. +++ b/core/modules/menu_ui/menu_ui.links.action.yml
   @@ -1,7 +1,7 @@
   +  add_destination: true
      appears_on:
        - entity.menu.edit_form
   

   This will need to be something like:

   options:
     add_destination: true
   

5. +++ b/core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
   @@ -2,6 +2,8 @@
   +@trigger_error('The ' . __NAMESPACE__ . '\MenuLinkAdd is deprecated in drupal:8.8.0. Instead, use `add_destination: true` parameter in declaration of action links.', E_USER_DEPRECATED);
   

   s/parameter/option/

   We will also need a change record for this, but don't worry about filing that, someone else can take care of it. Just pointing it out.

6. +++ b/core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
   @@ -10,6 +12,9 @@
   + * @deprecated in drupal:8.8.0. Instead, use `add_destination: true` parameter
   

   s/parameter/option/

7. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -275,10 +275,22 @@ public function doMenuTests() {
   +    // Test destination parameter in the url.
   

   Supernit: 'url' --> URL

8. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -275,10 +275,22 @@ public function doMenuTests() {
   +    $this->assertUrl('admin/structure/menu/manage/' . $menu_name . '/add?destination=' . $destination);
   

   Looks like assertUrl() is deprecated, it seems we should use $this->assertSession()->addressEquals() instead.

9. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -275,10 +275,22 @@ public function doMenuTests() {
   +    // Test destination redirect.
   +    $this->assertUrl($destination);
   

   Same here.

@phenaproxima Thanks for the quick review!

Here are the updates based on your comments.

Thanks! Changes look good. Since this change impacts a core class that's outside of any particular module's namespace, marking for framework manager review.

Fixing tests.

The test is probably failing because LocalActionDefaultTest needs the new parameter passed in setupLocalActionDefault.

1. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -38,10 +46,15 @@ class LocalActionDefault extends PluginBase implements LocalActionInterface, Con
   +  public function __construct(array $configuration, $plugin_id, $plugin_definition, RouteProviderInterface $route_provider, RedirectDestinationInterface $redirect_destination) {
   

   The new $redirect_destination parameter still needs to be documented on the method.

2. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -275,10 +275,22 @@ public function doMenuTests() {
   +    $destination = Url::fromRoute('entity.menu.edit_form', ['menu' => $menu_name])->toString();
   

   We should probably not use Url::fromRoute()->toString() since that adds the subdirectory. It is probably also why the test fails.

We cross posted, seems everything is addressed. Let's see if the tests pass and wait for framework manager review.

Fixing tests.

Tweaking tests.

Running single test to confirm it's passing.

I think this is the good one :)

I think this looks pretty good, honestly! Nice work!

1. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -275,10 +275,19 @@ public function doMenuTests() {
   +    $this->assertSession()->linkByHrefExists('admin/structure/menu/manage/' . $menu_name . '/add?destination=' . Url::fromRoute('entity.menu.edit_form', ['menu' => $menu_name])->toString(), 0, "The add menu link button URL does not have destination parameter");
   

   Nit: It looks like we use the Url::fromRoute('entity.menu.edit_form', ['menu' => $menu_name'])->toString() URL multiple times in the test. Can we do something like this, for clarity:

   $expected_destination = Url::fromRoute('entity.menu.edit_form', ['menu => $menu])->toString();
   $this->assertSession()->linkByHrefExists("admin/structure/menu/manage/$menu_name/add?destination=$expected_destination");
   

2. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -275,10 +275,19 @@ public function doMenuTests() {
   +    // Clink on the link.
   

   "Clink"? :)

3. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -275,10 +275,19 @@ public function doMenuTests() {
   +    // Create link.
   

   Nit: I don't think comment adds much :)

4. +++ b/core/modules/menu_ui/tests/src/Functional/MenuUiTest.php
   @@ -275,10 +275,19 @@ public function doMenuTests() {
        $this->assertUrl(Url::fromRoute('entity.menu.edit_form', ['menu' => $menu_name]));
   

   If we do the thing I suggested above, this can become $this->assertUrl($expected_destination).

@phenaproxima Thanks for the review!
I can surely put destination in the variable (that is how i initially did it, but then removed it :D )

However, point 4. would not work with the variable in that way, because toString() will actually prepend the subdirectory, and it will cause test to fail. The only way to pass this line is to use the URL object.
If you like the idea of using variable on multiple places, then we can do something like this

$expected_destination = Url::fromRoute('entity.menu.edit_form', ['menu => $menu]);
$this->assertSession()->linkByHrefExists("admin/structure/menu/manage/$menu_name/add?destination=" . $expected_destination->toString());
$this->assertUrl($expected_destination);

Here is a patch with fixes things mentioned in #32

Interdiff looks right; RTBC once it passes tests. Thanks, @botanic_spark!

1. The issue summary needs an update with what the solution is. Also the title scared me as I thought this was going to add redirects to all local tasks.
2. The idea looks really nice and this type of type helps to make a cohesive UI so a general +1

3. +++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
   @@ -119,7 +135,13 @@ public function getRouteParameters(RouteMatchInterface $route_match) {
   +    // Check if destination should be appended to the link.
   +    if (!empty($options['add_destination'])) {
   +      $options['query']['destination'] = $this->redirectDestination->get();
   +    }
   +    return $options;
   

   We need a change record to tell developers about the new option. We also should check core because there must some documentation somewhere about all the options in the menu_ui.links.action.yml files.

4. +++ b/core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
   @@ -2,6 +2,8 @@
   +@trigger_error('The ' . __NAMESPACE__ . '\MenuLinkAdd is deprecated in drupal:8.8.0. Instead, use `add_destination: true` option in declaration of action links.', E_USER_DEPRECATED);
   +
    use Drupal\Core\Menu\LocalActionDefault;
   

   Plugins aren't deprecated like this - we deprecate them in the constructor. This is because we only want to trigger on construction and not discovery.

5. +++ b/core/modules/menu_ui/menu_ui.links.action.yml
   @@ -1,7 +1,8 @@
   +  options:
   +    add_destination: true
      appears_on:
        - entity.menu.edit_form
   

   I wonder about the name "add_destination" - I know that you mean to return to the route that the action appears on when the user presses a button that triggers a form submission but maybe not. Also maybe it would be even more useful if it was something like destination: entity.menu.edit_form so you could a completely different destination.

Addressing #36:

1. Updated the issue summary with remaining tasks, also updated the title.
3. Found documentation in core in menu.api.php which seems relevant.
4. Needs work.
5. Needs discussion.

Related: #2762131: Generalize MenuLinkAdd so all local actions can use it

Changing description. And +1 for @alexpott #36.5 idea to have this option take the form of: destination: some.route_name.

I've created a module for normal menu links and wanted to extend it also to tasks, actions, etc. https://www.drupal.org/project/menu_link_destination

I wonder if that could be extended to cover all types of links and a candidate to be ported in core.

However, I think, the core feature should cover all types of links (menu, actions, tasks, etc)

Attempt at a re-roll here against 9.3.x, with the deprecation code moved into the constructor per #36.4.

Re: #36.5 (custom destinations): I looked into this and realized it would add some complexity to my use case. To get back to the current page, I would need to supply not only a route name, but also a route parameter with the current node (eg. local action is displaying on node/123/bar). If we decide to try for destination customization, then we'll probably need to figure out how to handle both the route name and route parameters. I somewhat lean toward the simplicity of the current add_destination: true approach, keeping in mind that it's still possible to achieve a custom destination by adding a class.

Re-roll against 10.1.x, but also applies to 9.5.x.

Trying to fix CCF errors, Facing some difficulties to create interdiff, So here i updating the patch alone.

Trying to fix CCF errors and add interdiff..

+++ b/core/lib/Drupal/Core/Menu/LocalActionDefault.php
@@ -37,11 +45,18 @@ class LocalActionDefault extends PluginBase implements LocalActionInterface, Con
+      @trigger_error('The redirect.destination service must be passed to LocalActionDefault::__construct(), it is required before Drupal 10.0.0.', E_USER_DEPRECATED);

+++ b/core/modules/menu_ui/src/Plugin/Menu/LocalAction/MenuLinkAdd.php
@@ -10,6 +10,10 @@
+ * @deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use the

@@ -35,6 +39,7 @@ class MenuLinkAdd extends LocalActionDefault {
+    @trigger_error("\Drupal\menu_ui\Plugin\Menu\LocalAction\MenuLinkAdd is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use the 'add_destination: true' option in declaration of action links.", E_USER_DEPRECATED);

deprecated in 10.1.0, removed from 11.0.0

Updated patch #51 by addressing #53, please review it.

Thanks & Regards,
Mrinalini

Reviewing for the 'Needs framework manager review' tag.

I would implement this differently

• Firstly, copy MenuLinkAdd to Drupal\Core\Menu\LocalActionWithRedirect or similar
• Secondly, gut MenuLinkAdd and have it extend the new class, and retain the deprecation error
• Then instead of adding a new property to the yaml, for the links that need this, just add class: Drupal\Core\Menu\LocalActionWithRedirect

This avoids needing to add a new property and just uses the plugin system we already have.

Thoughts?

+1 to the idea in #55.

Like the idea of #55 also.

Moving to NW for those changes. Also issue summary should be updated to match new proposal.

Hi!

I like the idea of #55 as using a class called LocalActionWithRedirect is more intuitive than MenuLinkAdd.

On the other hand, in the documentation (a bit outdated), it is written in both menu.links and local.tasks examples of how to use the 'options' within the yml, and:

  options:
    query:
      destination: '/path/to/route'

can be defined if we need a destination different than the same route (which is added by default when using the class).

But nothing is documented in the links.actions page

Best

Yes, per #59 if you do this code below then the current implementation of MenuLinkAdd will clobber this.

/**
 * Implements hook_menu_local_actions_alter(),
 */
function MYMODULE_menu_local_actions_alter(&$local_actions) {
  if (isset($local_actions['foo.bar'])) {
    $local_actions['foo.bar']['options']['query']['destination'] = '/cheese';
  }
}

I think I like this approach better but think this was fixed by #2762131: Generalize MenuLinkAdd so all local actions can use it so we can close this.

Saving credits according to our core issue credit guidelines. Thanks!

(Remember, closed issues get credits now, so any time you mark something wontfix/duplicate/etc., you're saving the credit records as they stood if the issue is from back when default credit was given for file attachments.)