Add destination to edit, delete, enable, disable links in entity list builders

Somewhat related: #2838968: BlockContentListBuilder should use RedirectDestinationTrait instead of hardcoding the collection link template

As commented in that issue, we need to use the redirect destination trait instead so it works with custom views pages and so on as well.

My recommendation would actually be to simply add the destination explicitly to the default edit and delete links and not all of them.

Simple example where a destination IMHO doesn't make sense is for examplethe field_ui links, I don't think those should return back and it might result in strange behavior on those forms. If we add it to the default edit/delete links then we cover the 80% use case and can IMHO even do so by default, without a new class, so that everyone benefits from it.

That's also consistent with the logic in \Drupal\node\NodeListBuilder::getDefaultOperations(), with what I'm doing in #2838968: BlockContentListBuilder should use RedirectDestinationTrait instead of hardcoding the collection link template as well as the one added in #2469567: Entity operations for terms are hardcoded in taxonomy terms's overview page.

This is what I had in mind.

Ha, ConfigurationTest in action.module was fun. Already ported to BrowserTestBase but still thinking like Drupal 7 where aid's where a thing and was a number that had to be parsed out of a URL. That regex also catched the ?destination that's now part of the URL and it resulted in a completely messed up URL. Which is not needed anyway, as we already click on the Delete link and already are on that page.

Simplifed that test.

Same story for \Drupal\node\Tests\NodeActionsConfigurationTest.

And some other fixes, didn't fix the views test yet, as I'm not sure what to do about this. This basically means that the destination can't be disabled unless views explicitly *removes* the destination key and I'm not sure why that would be useful, honestly.

About the (array) thing, mergeOptions() actually landed a few days ago, which makes this way simpler, even wondering to drop ensureDestination() again and just call this directly.

That's true, but not sure if that's really a problem, afaik only it would already be returned by toUrl(), which seems pretty unlikely?

About the views setting, not sure. It might still affect some other operations but the question is if you'd want those to have a destination anyway?

Happy to hear use cases from Xano if he thinks that there is a reason. *Adding* that option made sense, as you wouldn't have wanted to not have those destinations. I see little reason to not have them, especially as we've now also fixed some related bugs with e.g. node preview. And as mentioned, the setting already didn't work for nodes, which is probably still the 80% use case for creating views with operation links...

Somewhat related #2668794: RedirectDestination:get doesn't use proper query parameters the same issue with the destination parameter exists for admin listings.

Somewhat related #2668794: RedirectDestination:get doesn't use proper query parameters the same issue with the destination parameter exists for admin listings.

One annoyance with the "out-of-the-box" experience when creating new entity types is the lack of redirects from add, edit and delete forms.

Yeah these are the "small" things which sum up as being annoying. Having them fixed is just great.

On mergeOptions(): I was thinking about that too, but AFAIK this will now overwrite a pre-existing destination query, whereas the previous patch did not. I may be missing something, though...

That's true, but not sure if that's really a problem, afaik only it would already be returned by toUrl(), which seems pretty unlikely?

I believe this is actually a fair price to pay. Especially when people really care, they could still implement their own list builder and override it there again.

About the views setting, not sure. It might still affect some other operations but the question is if you'd want those to have a destination anyway?

I'm pretty sure this option wasn't added 100% on purpose but rather this was about having some kind of parity with \Drupal\views\Plugin\views\field\Links

All in all I would be personally be fine with +1 on removing the option from views.

Not every operation and not every entity type will include a destination. For example translate will not and I think that actually makes sense, as the 90% use case there is to go to another page and e.g. the Add/Edit buttons there lose the destination anyway.

What about adding a description to that setting that says something like: "Force a destination for every operation. Many operations will already include the correct destination, it is recommended to have this disabled unless needed." Or maybe even put a "Deprecated:" prefix in front of it?

And then figure out what to do about the tests :)

Reroll for now.

The views test was actually just broken because it uses string concatenation and ended up with URL?destination=/?destination=... ;)

Implemented my suggestion from above with the updated description. also disabled the setting by default. Wondering if and what that is going to break.

+++ b/core/modules/filter/src/Tests/FilterAdminTest.php
@@ -135,16 +136,9 @@ public function testFormatAdmin() {
-    // Cannot use the assertNoLinkByHref method as it does partial url matching
-    // and 'admin/config/content/formats/manage/' . $format_id . '/disable'
-    // exists.
-    // @todo: See https://www.drupal.org/node/2031223 for the above.
-    $edit_link = $this->xpath('//a[@href=:href]', [
-      ':href' => \Drupal::url('entity.filter_format.edit_form', ['filter_format' => $format_id])
-    ]);
-    $this->assertTrue($edit_link, format_string('Link href %href found.',
-      ['%href' => 'admin/config/content/formats/manage/' . $format_id]
-    ));
+    $destination = Url::fromRoute('filter.admin_overview')->toString();
+    $edit_href = Url::fromRoute('entity.filter_format.edit_form', ['filter_format' => $format_id], ['query' => ['destination' => $destination]])->toString();
+    $this->assertLinkByHref($edit_href);

This is some nice improvement

Yeah, with the destination, the links are now unique and we longer need the custom asserts.

However, I was a bit confused with that comment because it says assertNoLinkByHref(), but we actually assert that there is one.

LGTM!

+++ b/core/lib/Drupal/Core/Entity/EntityListBuilder.php
@@ -250,4 +254,17 @@ protected function getTitle() {
+   *   The URL object.

Nit: "The updated URL object."

No longer applies

+++ b/core/modules/node/src/NodeListBuilder.php
@@ -41,14 +33,11 @@ class NodeListBuilder extends EntityListBuilder {
-   * @param \Drupal\Core\Routing\RedirectDestinationInterface $redirect_destination
-   *   The redirect destination service.
    */
-  public function __construct(EntityTypeInterface $entity_type, EntityStorageInterface $storage, DateFormatterInterface $date_formatter, RedirectDestinationInterface $redirect_destination) {
+  public function __construct(EntityTypeInterface $entity_type, EntityStorageInterface $storage, DateFormatterInterface $date_formatter) {
     parent::__construct($entity_type, $storage);
 
     $this->dateFormatter = $date_formatter;
-    $this->redirectDestination = $redirect_destination;

@@ -58,8 +47,7 @@ public static function createInstance(ContainerInterface $container, EntityTypeI
-      $container->get('date.formatter'),
-      $container->get('redirect.destination')
+      $container->get('date.formatter')

This change is unnecessary in my book.

Yes, we can get it from the trait, but that doesn't preclude injecting it properly.

And removing this change makes the patch less disruptive.

but +1 on this change

Rerolled and restored the constructor argument in NodeListBuilder, fine with keeping that.

+++ b/core/modules/views/src/Plugin/views/field/EntityOperations.php
@@ -99,7 +99,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
+      ... Most operations include a destination by default and this setting is not longer needed ...

Not longer needed => No longer needed, can be fixed on commit

Can we get a change record here - thanks - there would be lots of contrib modules that can benefit from this

This enough: https://www.drupal.org/node/2904582?

Manually tested, works like it says on the box.

Committed as 401bd66 and pushed to 8.5.x. Thanks! >

Published change record.

Re-rolled the patch from #32 to apply on 8.4.1. Just in case anyone needs it.