Simplify custom_block_menu_local_tasks_alter(). [#1919926]

Follow up for #1871772-108: Convert custom blocks to content entities

Problem/Motivation

+++ b/core/modules/block/custom_block/custom_block.module
@@ -5,29 +5,129 @@
+function custom_block_menu_local_tasks_alter(&$data, $router_item, $root_path) {
...
+ if (substr($root_path, 0, 26) == 'admin/structure/block/list') {
...
+ $path_map = explode('/', $root_path);
I just left some remarks in IRC that this implementation of hook_menu_local_tasks_alter() is one of the most complex I've ever seen and that I'm absolutely confident that it can be vastly simplified.

But that's straight refactoring/simplification, so can happen later. :)

That said, in case #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage will land first, this hook needs to be adjusted to be not an alter hook, since it adds a local task (instead of altering an existing).

Proposed resolution

Refactor custom_block_menu_local_tasks_alter() once #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage lands

Remaining tasks

Write patch

Comment	File	Size	Author
#7	drupal8.custom-block-local-action.7.patch	4.34 KB	sun
#7
#3	Screen Shot 2013-02-19 at 4.15.01 PM.png	18.84 KB	larowlan
#3	Screen Shot 2013-02-19 at 4.15.17 PM.png	17.32 KB	larowlan
#3	Screen Shot 2013-02-19 at 4.15.24 PM.png	23.63 KB	larowlan
#3	custom-block-local-tasks-1919926.3.patch	2.43 KB	larowlan
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

sun

German

Karlsruhe

CreditAttribution: sun commented 19 February 2013 at 05:40

Category:	task	» bug
Status:	Postponed	» Active
Issue tags:	-Entity system, -D8 upgrade path

Since #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage landed in parallel, the current code is definitely broken, as the structure of $data was changed, too.

Comment #2

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 19 February 2013 at 06:05

Assigned:

Unassigned

» larowlan

working on it now

Comment #3

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 19 February 2013 at 06:26

Status:

Active

» Needs review

File	Size
custom-block-local-tasks-1919926.3.patch	2.43 KB

Screen Shot 2013-02-19 at 4.15.24 PM.png	23.63 KB
Screen Shot 2013-02-19 at 4.15.17 PM.png	17.32 KB
Screen Shot 2013-02-19 at 4.15.01 PM.png	18.84 KB

Refactors to use hook_menu_local_tasks() and removes a lot of the logic in favour of preg_match().
Shot from seven (shown as expected)
Screen Shot 2013-02-19 at 4.15.01 PM.png
Shot from bartik (Default theme) (shown as expected)

Shot from other items with same tab root (not shown as expected)

We already had tests for the presence of the link.
Added another to make sure it doesn't show up on other items under tab root.

Comment #4

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 19 February 2013 at 06:27

Assigned:

larowlan

» Unassigned

Comment #5

sun

German

Karlsruhe

CreditAttribution: sun commented 19 February 2013 at 07:50

+++ b/core/modules/block/custom_block/custom_block.module
@@ -11,33 +11,17 @@
   // Make sure we're at least in the right general area.
...
+  if (preg_match('@^admin/structure/block/list/(.*)/add$@', $root_path)) {

This still looks a bit too complex to me.

Can you clarify which exact paths we're trying to match? And which ones we don't want to match?

Comment #6

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 19 February 2013 at 08:14

Hey @sun

admin/structure/block/list/block_plugin_ui%3Abartik/add
admin/structure/block/list/block_plugin_ui%3Aseven/add
admin/structure/block/list/block_plugin_ui%3Astark/add

Basically admin/structure/block/list/block_plugin_ui:$theme_name/add for any theme name

Comment #7

sun

German

Karlsruhe

CreditAttribution: sun commented 19 February 2013 at 09:01

File	Size
drupal8.custom-block-local-action.7.patch	4.34 KB

OK, now I understand what's going on. I've added @todos to clarify why this is unnecessarily complex.

For now I referenced #1067408: Themes do not have an installation status since one of its changes is to resolve exactly this. However, we might want to create a separate, dedicated issue for this.

I also removed the 'output' subkey in the returned action array, since that is no longer needed due to #1864066: Simplify hook_menu_local_tasks() and MENU_DEFAULT_LOCAL_TASK usage