Problem/Motivation
Currently local actions do not provide a destination which results in an inconsistent UX because users expect to return to the page where they clicked the button.
Steps to reproduce:
- Sign in as an administrator
- Go to admin/people
- Click the 'Add user' local action
- Fill out the form to add a new user.
- You get 'stuck' on the Add user page, when most people would have wanted to return to the People listing.
Proposed resolution
Provide a local action option parameter so the user is 'returned' to the page from where the click on the action link originated.
Remaining tasks
Fix deprecation (#36 point 4)- Expand documentation in menu.api.php
- Expand functionality allowing to set a destination? (see #36 point 5)
- Review
- Create change record
User interface changes
When the option is provided in a specific local action *.links.action.yml, the user who clicks the action will be redirected to the destination parameter path. @alexpott in comment #36.5 proposes that the destination route could be specified in the yml entry.
API changes
There is a new option 'add_destination' available for local action plugins. @alexpott in comment #36.5 proposes that option should have the form: destination: some.route_name
allowing developers to specify any route for the destination.
Data model changes
None.
Release notes snippet
Needs change record.
Comment | File | Size | Author |
---|---|---|---|
#54 | interdiff_51-54.txt | 2.26 KB | mrinalini9 |
#54 | 1344902-54.patch | 9.92 KB | mrinalini9 |
| |||
#51 | interdiff_50-51.txt | 626 bytes | Nikhil_110 |
#51 | 1344902-51.patch | 9.91 KB | Nikhil_110 |
|
Comments
Comment #1
Dave ReidThis can be implemented in a custom module with the following code:
Comment #5
joelstein CreditAttribution: joelstein at On Fire Media commentedI 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...
Comment #6
joelstein CreditAttribution: joelstein at On Fire Media commentedActually, 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:
Comment #9
gordon CreditAttribution: gordon at Heydon Consulting commentedLooking 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.
Comment #11
seanB#3033960: After adding media, return the user to the media listing they started from is very much related.
Comment #12
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedThe 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.
Comment #13
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedComment #14
phenaproximaThis 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.
Needs to be protected.
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.
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.
This will need to be something like:
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.
s/parameter/option/
Supernit: 'url' --> URL
Looks like assertUrl() is deprecated, it seems we should use $this->assertSession()->addressEquals() instead.
Same here.
Comment #15
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commented@phenaproxima Thanks for the quick review!
Here are the updates based on your comments.
Comment #16
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedComment #17
phenaproximaThanks! Changes look good. Since this change impacts a core class that's outside of any particular module's namespace, marking for framework manager review.
Comment #20
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedFixing tests.
Comment #21
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedComment #22
seanBThe test is probably failing because
LocalActionDefaultTest
needs the new parameter passed insetupLocalActionDefault
.The new $redirect_destination parameter still needs to be documented on the method.
We should probably not use
Url::fromRoute()->toString()
since that adds the subdirectory. It is probably also why the test fails.Comment #23
seanBWe cross posted, seems everything is addressed. Let's see if the tests pass and wait for framework manager review.
Comment #25
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedFixing tests.
Comment #26
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedComment #28
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedTweaking tests.
Comment #29
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedRunning single test to confirm it's passing.
Comment #30
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedI think this is the good one :)
Comment #31
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedComment #32
phenaproximaI think this looks pretty good, honestly! Nice work!
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:
"Clink"? :)
Nit: I don't think comment adds much :)
If we do the thing I suggested above, this can become $this->assertUrl($expected_destination).
Comment #33
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commented@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
Comment #34
botanic_spark CreditAttribution: botanic_spark at FFW, Develomon commentedHere is a patch with fixes things mentioned in #32
Comment #35
phenaproximaInterdiff looks right; RTBC once it passes tests. Thanks, @botanic_spark!
Comment #36
alexpottWe 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.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.
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.Comment #37
askibinski CreditAttribution: askibinski at ezCompany for Erasmus University Rotterdam commentedAddressing #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.
Comment #38
askibinski CreditAttribution: askibinski at iO for Erasmus University Rotterdam commentedRelated: #2762131: Generalize MenuLinkAdd so all local actions can use it
Comment #42
lunk rat CreditAttribution: lunk rat commentedChanging description. And +1 for @alexpott #36.5 idea to have this option take the form of:
destination: some.route_name
.Comment #44
claudiu.cristeaI'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)
Comment #45
chrisolof CreditAttribution: chrisolof at Greentech Renewables commentedAttempt 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 currentadd_destination: true
approach, keeping in mind that it's still possible to achieve a custom destination by adding a class.Comment #49
DieterHolvoet CreditAttribution: DieterHolvoet at Minsky commentedRe-roll against 10.1.x, but also applies to 9.5.x.
Comment #50
sahil.goyal CreditAttribution: sahil.goyal as a volunteer and for Drupal India Association commentedTrying to fix CCF errors, Facing some difficulties to create interdiff, So here i updating the patch alone.
Comment #51
Nikhil_110 CreditAttribution: Nikhil_110 at Srijan | A Material+ Company commentedTrying to fix CCF errors and add interdiff..
Comment #52
Nikhil_110 CreditAttribution: Nikhil_110 at Srijan | A Material+ Company commentedComment #53
andypostdeprecated in 10.1.0, removed from 11.0.0
Comment #54
mrinalini9 CreditAttribution: mrinalini9 at Srijan | A Material+ Company for Drupal India Association commentedUpdated patch #51 by addressing #53, please review it.
Thanks & Regards,
Mrinalini
Comment #55
larowlanReviewing for the 'Needs framework manager review' tag.
I would implement this differently
class: Drupal\Core\Menu\LocalActionWithRedirect
This avoids needing to add a new property and just uses the plugin system we already have.
Thoughts?
Comment #56
heddn+1 to the idea in #55.
Comment #57
smustgrave CreditAttribution: smustgrave at Mobomo commentedLike the idea of #55 also.
Moving to NW for those changes. Also issue summary should be updated to match new proposal.
Comment #59
gorkagr CreditAttribution: gorkagr commentedHi!
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:
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
Comment #60
simeYes, per #59 if you do this code below then the current implementation of
MenuLinkAdd
will clobber this.