Problem/Motivation

When working with the Block Layout interface, it's confusing to delete a block and be taken back to another theme's block layout page. This could cause users to accidentally delete blocks from another theme (thinking they belong to the theme they just deleted a block from).

Proposed resolution

This might not be the Drupal 8 way of doing things (since I just started diving into Drupal 8 Core recently), but for now I suggest we just add a destination query string variable to the delete operation that returns to the current page.

Remaining tasks

Attach the patch.

User interface changes

The interface itself does not change, it just redirects to a different location.

API changes

There are no API changes.

Data model changes

There are no Data model changes.

CommentFileSizeAuthor
#39 interdiff-36-39.txt1.2 KBkrzysztof domański
#39 core_2527628-39.patch2.81 KBkrzysztof domański
#36 interdiff-35-36.txt2.32 KBkrzysztof domański
#36 core_2527628-36.patch2.77 KBkrzysztof domański
#35 interdiff-32-35.txt1.92 KBkrzysztof domański
#35 core_2527628-35.patch2.67 KBkrzysztof domański
#32 interdiff-30-32.txt2.59 KBkrzysztof domański
#32 core_2527628-32.patch2.59 KBkrzysztof domański
#30 core_2527628-30.patch2.04 KBkrzysztof domański
#28 core_2527628-28.patch2.52 KBkrzysztof domański
#21 2527628-21.drupal.block-delete-redirect.patch3.26 KBjoachim
#18 2527628-18.drupal.block-delete-redirect.patch2.36 KBjoachim
#16 2527628-16.drupal.block-delete-redirect.patch2.36 KBjoachim
#13 2527628-redirect-13.patch2.54 KBswentel
#9 interdiff.txt591 bytestim.plunkett
#9 2527628-redirect-9.patch2.54 KBtim.plunkett
#8 2527628-redirect-8.patch2.53 KBtim.plunkett
#1 block_delete_redirect-2527628-1.patch892 bytessammarks15
#5 2527628-redirect-5-PASS.patch4.49 KBtim.plunkett
#5 2527628-redirect-5-FAIL.patch1.47 KBtim.plunkett

Issue fork drupal-2527628

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

sammarks15’s picture

sammarks15 commented
Version: 8.0.x-dev » 8.0.0-beta12
StatusFileSize
new block_delete_redirect-2527628-1.patch892 bytes

Here's the patch that introduces the changes.

I just noticed the change is actually in the EntityListBuilder class. This shouldn't cause any problems with other parts of Core (since redirecting back to the page the user was on is the expected functionality in most cases), but I'm open to suggestions.

I also updated the version to beta12, as that's the version I based the patch off of (I couldn't get the latest development version to run for testing).

sammarks15’s picture

sammarks15 commented
Status: Active » Needs review

I noticed that the patch, when applied, causes one of the tests to fail. Could someone explain why I'm getting an error with the dependency injection? Here's the error that occurs in the test:

Drupal\Tests\Core\Entity\EntityListBuilderTest::testGetOperations
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "url_generator" does not exist.

It stems from line 127 of core/tests/Drupal/Tests/Core/Entity/EntityListBuilderTest.php

Status: Needs review » Needs work

The last submitted patch, 1: block_delete_redirect-2527628-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Version: 8.0.0-beta12 » 8.0.x-dev
Issue tags: +Needs tests

Good find! We should add automated tests for this.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs work » Needs review
Issue tags: -Needs tests
StatusFileSize
new 2527628-redirect-5-FAIL.patch1.47 KB
new 2527628-redirect-5-PASS.patch4.49 KB

I think instead of hardcoding 'current' we should consult the service, otherwise we'll override any current ?destination.
I've added test coverage for both cases.

Also note that the node and user listings already handled this.
Should we opt all entities into it, or just have block do it as well?

The last submitted patch, 5: 2527628-redirect-5-PASS.patch, failed testing.

The last submitted patch, 5: 2527628-redirect-5-FAIL.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 2527628-redirect-8.patch2.53 KB

I don't think it's appropriate to modify ALL of the entity list builders. They can opt-into this themselves.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
StatusFileSize
new 2527628-redirect-9.patch2.54 KB
new interdiff.txt591 bytes 
+++ b/core/modules/block/src/BlockListBuilder.php
@@ -381,6 +384,9 @@ public function getDefaultOperations(EntityInterface $entity) {
+    if (isset($operations['delete'])) {
+      $operations['edit']['query'] = $this->getDestinationArray();

Ughhh sloppy copy/paste (delete vs edit)

sammarks15’s picture

sammarks15 commented

Completely agree with not automatically opting all entity list builders into it. I would have done that myself, except I didn't know the correct Drupal 8 way to do it :)

The last submitted patch, 8: 2527628-redirect-8.patch, failed testing.

mgifford’s picture

mgifford
he/him
Primary language English
commented
Status: Needs review » Needs work

Patch needs re-roll.

swentel’s picture

swentel commented
Status: Needs work » Needs review
StatusFileSize
new 2527628-redirect-13.patch2.54 KB

Got bitten by this one too - rerolled

chi’s picture

chi commented

Is it worth to inject redirect.destination service into the BlockListBuilder instead of using the trait?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

joachim’s picture

joachim commented
Version: 8.1.x-dev » 8.2.x-dev
Issue tags: +Site builder experience (SX)
Related issues: +#2712397: Deleting a block from a theme that's not the default redirects me to the default theme blocks list
StatusFileSize
new 2527628-16.drupal.block-delete-redirect.patch2.36 KB

BlockDeleteForm already implements getCancelUrl(), so I think it's more elegant to return the right URL here than use a query destination string.

(It's getCancelUrl() rather than getRedirectUrl() because of #2712417: EntityDeleteFormTrait doesn't use its own getRedirectUrl().)

Status: Needs review » Needs work

The last submitted patch, 16: 2527628-16.drupal.block-delete-redirect.patch, failed testing.

joachim’s picture

joachim commented
Status: Needs work » Needs review
StatusFileSize
new 2527628-18.drupal.block-delete-redirect.patch2.36 KB

Weird. Tests pass for me locally.

Here's a reroll.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented 
  1. +++ b/core/modules/block/src/Form/BlockDeleteForm.php
@@ -14,7 +14,16 @@ class BlockDeleteForm extends EntityDeleteForm {
+    $default_theme = \Drupal::service('theme_handler')->getDefault();

    This should inject the service. 

  2. +++ b/core/modules/block/src/Form/BlockDeleteForm.php
@@ -14,7 +14,16 @@ class BlockDeleteForm extends EntityDeleteForm {
+    if ($block_theme == $default_theme) {

    Might as well use ===

Status: Needs review » Needs work

The last submitted patch, 18: 2527628-18.drupal.block-delete-redirect.patch, failed testing.

joachim’s picture

joachim commented
Status: Needs work » Needs review
StatusFileSize
new 2527628-21.drupal.block-delete-redirect.patch3.26 KB

Done both of those.

(In true XKCD condiment-passing style, this is been the spur I needed to add support for injecting any service into forms generated by Module Builder!)

Status: Needs review » Needs work

The last submitted patch, 21: 2527628-21.drupal.block-delete-redirect.patch, failed testing.

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.

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
Status: Needs work » Needs review
Related issues: +#2980527: Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error
StatusFileSize
new core_2527628-28.patch2.52 KB

Tested manually. Currently, deleting a block redirect back to the correct theme! The problem was last reported in the version 8.2.

I added a patch with test coverage.

Notice: Not works when using domain language negotiation but there is an additional issue:
#2980527: Domain-based language negotiation strips "destination" URL query argument, causing BigPipe error

Status: Needs review » Needs work

The last submitted patch, 28: core_2527628-28.patch, failed testing. View results

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
Status: Needs work » Needs review
StatusFileSize
new core_2527628-30.patch2.04 KB

Status: Needs review » Needs work

The last submitted patch, 30: core_2527628-30.patch, failed testing. View results

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
Status: Needs work » Needs review
StatusFileSize
new core_2527628-32.patch2.59 KB
new interdiff-30-32.txt2.59 KB

Improving test
Url::fromRoute('block.admin_display') instead 'admin/structure/block'
Url::fromRoute('block.admin_display_theme', ['theme' => 'seven']) instead 'admin/structure/block/list/seven'

Status: Needs review » Needs work

The last submitted patch, 32: core_2527628-32.patch, failed testing. View results

krzysztof domański’s picture

krzysztof domański
Location Poland
commented

The last submitted patch, 32: core_2527628-32.patch, failed testing.

Current page is "/subdirectory/admin/structure/block", but "/admin/structure/block" expected.

Similar problem #2986962: BrowserTestBase::drupalGet() does not appear to be handling base url properly

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
Status: Needs work » Needs review
StatusFileSize
new core_2527628-35.patch2.67 KB
new interdiff-32-35.txt1.92 KB
krzysztof domański’s picture

krzysztof domański
Location Poland
commented
StatusFileSize
new core_2527628-36.patch2.77 KB
new interdiff-35-36.txt2.32 KB

Refactoring of deprecated code.

   * @deprecated Scheduled for removal in Drupal 9.0.0.
   *   Use $this->assertSession()->responseContains() instead.
   */
  protected function assertRaw($raw) {

The last submitted patch, 35: core_2527628-35.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 36: core_2527628-36.patch, failed testing. View results

krzysztof domański’s picture

krzysztof domański
Location Poland
commented
Status: Needs work » Needs review
StatusFileSize
new core_2527628-39.patch2.81 KB
new interdiff-36-39.txt1.2 KB

Status: Needs review » Needs work

The last submitted patch, 39: core_2527628-39.patch, failed testing. View results

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.

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.

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.

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.

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.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented
Priority: Normal » Minor
Issue tags: +Bug Smash Initiative, +Needs reroll
bhanu951’s picture

bhanu951 commented
Assigned: Unassigned » bhanu951

Bhanu951 opened merge request !2994

bhanu951’s picture

bhanu951 commented
Assigned: bhanu951 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs reroll
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Thank you @Bhanu951 for the MR.

Appear to be some failures in the tests. Moving back to NW for those.

smustgrave’s picture

smustgrave commented
Issue tags: +Needs issue summary update

This will need an issue summary update and title update. If the issue is fixed and this is just about adding test coverage then it should be stated.

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.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.