Updated: Comment #0

Problem/Motivation

  • Following #2062439: Provide listing of custom block entities and #2058321: Move the 'place block' UI into the block listing, there are two entry points for creating a new custom block: The custom block listing at admin/structure/custom-blocks, and the block placement form at admin/structure/block.
  • When you add a custom block from admin/structure/block, submitting the custom block creation form takes you directly into the "place block" form for an instance of this new block, and then subsequently redidirects back to the place block form.
  • When you add a custom block from admin/structure/custom-blocks, the behavior changes based on whether you have only one custom block type:
    • If you have only one custom block type, you are redirected back to admin/structure/custom-blocks, as you'd expect.
    • If you have multiple custom block types, you first select your block type, then fill out the custom block form... and then are unexpectedly redirected into the place block workflow and then to admin/structure/block, despite that you might not have any intention of placing the block in the first place.

Video illustrating this adventure:
https://www.dropbox.com/s/b15zhahytrww9sn/custom_block_redirect.mp4

Proposed resolution

Always redirect back to admin/structure/block/custom-blocks if that's where you start.

Additionally, if you started on a theme-specific page like admin/structure/block/list/seven, you will be redirected there.

Remaining tasks

Remove the seven theme override of the custom block list theming (ie this works in stark)

User interface changes

The user is consistently redirected to admin/structure/custom-blocks if that's the starting point.

API changes

Probably none.

Files: 
CommentFileSizeAuthor
#24 custom-blocks-2068053-24.patch9.66 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,338 pass(es).
[ View ]
#22 custom-blocks-2068053-22.patch9.65 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,725 pass(es).
[ View ]
#20 interdiff.txt10.68 KBtim.plunkett
#20 custom-blocks-2068053-20-FAIL.patch5.34 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 59,017 pass(es), 27 fail(s), and 7 exception(s).
[ View ]
#20 custom-blocks-2068053-20-PASS.patch10.61 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 59,017 pass(es).
[ View ]
#15 block-2068053-15.patch4.23 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 58,477 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#8 custom-block-redirect.patch965 byteslarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,851 pass(es).
[ View ]

Comments

Status:Active» Closed (duplicate)

Status:Closed (duplicate)» Active

Apparently it wasn't, according to xjm.

Component:configuration system» custom_block.module
Issue tags:+Needs tests

Grr you can see the destination in the URL on the list screen but either it's not in the links or overlay stuffs it. Hoping it's the latter cause the tests do exactly what the video does. The only difference is overlay.

Nope doesn't work. Has to be seven.

I'm not sure which is stranger, seven_custom_block_add_list completely bypassing the preprocess, or custom-block-add-list.html.twig using the "node-type-list" class :)

Good find!

Status:Active» Needs review
StatusFileSize
new965 bytes
PASSED: [[SimpleTest]]: [MySQL] 57,851 pass(es).
[ View ]

So in terms of 'needs tests' do we need to duplicate the tests with the seven theme enabled </groan>

Can't we just gut/remove the seven override? Also, put the test into a helper method and run it twice...

Status:Needs review» Reviewed & tested by the community

I can't comment on tim.plunkett's idea in #10, but after applying the patch in #8 by larowlan, I can confirm that desired redirect effect happens.
Here is a video of after the patch: http://www.youtube.com/watch?v=8PaGHF110Pg

Status:Reviewed & tested by the community» Needs work

This still needs tests.

Agreed approach: remove the override in seven

Assigned:Unassigned» tim.plunkett

Working on this.

Status:Needs work» Needs review
Issue tags:-Usability, -Needs tests
StatusFileSize
new4.23 KB
FAILED: [[SimpleTest]]: [MySQL] 58,477 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Well this is fun!

CustomBlockInterface::getTheme() and CustomBlockInterface::setTheme() are never actually used.
Or, that is, setTheme() is never used.

So this isn't even really related to the OP, but is covered under the title.

Status:Needs review» Needs work
Issue tags:+Usability, +Needs tests

The last submitted patch, block-2068053-15.patch, failed testing.

Current URL is 'http://ns4007744.ip-192-95-30.net/checkout/admin/structure/block/add/custom_block%3A35922bf3-3d37-4bed-8502-2a192c94b332/seven'.
Current URL is 'http://ns4007744.ip-192-95-30.net/checkout/admin/structure/block/add/custom_block%3Ae570ce8e-e2b9-4227-9105-5d711f7568ce/bartik'.

Those are wrong because it always 'stark' due to getTheme() always returning NULL because setTheme() is never called:

CustomBlockFormController:

        if ($theme = $block->getTheme()) {
          $form_state['redirect'] = 'admin/structure/block/add/custom_block:' . $block->uuid->value . '/' . $theme;
        }
        else {
          $form_state['redirect'] = 'admin/structure/block/add/custom_block:' . $block->uuid->value . '/' . \Drupal::config('system.theme')-          >get('default');
        }

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/Controller/CustomBlockController.php
@@ -106,6 +106,7 @@ public function addForm(CustomBlockTypeInterface $custom_block_type, Request $re
     if (($theme = $request->attributes->get('theme')) && in_array($theme, array_keys(list_themes()))) {
+      throw new \Exception('Betcha this code will never fire!');
       // We have navigated to this page from the block library and will keep track
       // of the theme for redirecting the user to the configuration page for the
       // newly created block in the given theme.
       $block->setTheme($theme);
     }

If we try to do something like block/add?theme=bartik, that breaks drupal_get_desintation (which is insane, btw).

So I don't have any obvious solutions on how to track the theme name from page to page...

We discussed this in irc and decided not to use drupal_get_destination

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new10.61 KB
PASSED: [[SimpleTest]]: [MySQL] 59,017 pass(es).
[ View ]
new5.34 KB
FAILED: [[SimpleTest]]: [MySQL] 59,017 pass(es), 27 fail(s), and 7 exception(s).
[ View ]
new10.68 KB

Okay, here we go!
We can't use a proper local action because we need a query string, so we fake it.
CustomBlockController::addForm() now checks $request->query
seven_custom_block_add_list() is not outright removed yet, but it is brought into line with template_preprocess_custom_block_add_list().

+++ b/core/modules/simpletest/lib/Drupal/simpletest/WebTestBase.php
@@ -2377,7 +2377,11 @@ protected function assertUrl($path, array $options = array(), $message = '', $gr
+    // Paths in query strings can be encoded or decoded with no functional
+    // difference, decode them for comparison purposes.
+    $actual_url = urldecode($this->getUrl());
+    $expected_url = urldecode($this->container->get('url_generator')->generateFromPath($path, $options));
+    return $this->assertEqual($actual_url, $expected_url, $message, $group);

By the way, I have this hunk in about 3 different issues, I've been doing a lot with query strings lately :) It's in #2083877: Add a theme selector to the place block form which is RTBC

StatusFileSize
new9.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,725 pass(es).
[ View ]

That went in.

Status:Needs review» Reviewed & tested by the community

Thanks @timplunkett for resolving this

StatusFileSize
new9.66 KB
PASSED: [[SimpleTest]]: [MySQL] 59,338 pass(es).
[ View ]

Issue summary:View changes

Updated issue summary.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

Assigned:tim.plunkett» Unassigned

Thank you!

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

Issue summary:View changes

Updated