hook_block_view_MODULE_DELTA_alter fails with menu blocks

Related:
http://drupal.org/node/1129972
http://drupal.org/node/1021270
http://drupal.org/node/992376

Duplicate of #1021270: Blocks for custom menus are impossible to theme

Actually, this is not at all duplicate.
That one was about the template suggestions not working this one is about hook implementations. The root of the problem is the same (hyphens in block deltas), but they need to be handled separately.
I'll see if I can roll a patch.

Here we go.

I tested this patch, and it works as advertised.
In theory, we could write tests for this, but this is pretty specific, and it's very unlikely that we'll ever break this again, so I would vote against it.

0.0

Thinking about this, I think we should swap the approach, at least for D8.
We don't replace hypens with underscores elsewhere, for form IDs, node types, etc.
We should standardize on hyphen-less block deltas instead, I think.
For D7, though, this patch fixes a bug, so is definitely valid.

Not to fix, but switch the hook_block_view_alter() can work correct.

Looking at the bright side, this will fix it for any module that uses hyphens in module deltas. Since hook_block_view_MODULE_DELTA_alter was broken for them, this won't do any harm, just bring lots of joy.

Thanks for your work on this. Regarding #7, we shouldn't fix this in D7 until it is fixed in D8 one way or another, because otherwise we'll have a regression. I'd suggest committing this fix to D8 and D7, and then opening a followup for the better approach in D8.

Also, this seems like something that could use an automated test.

I suspect that I'm going to be overruled, but I think this is one of those things where it doesn't really make sense to write a test. When you think about it, menu.module is misusing the block API, it just took us a couple versions to figure that out. In D8, as I said, we should fix this proper (in menu.module), so those tests would be superfluous anyway. We would be testing for an obscure edge-case bug which we only support for BC, not because it's actually a feature.

Well, if we're going to support it for BC and it's an extreme edge case, all the more reason to have a test to make sure we don't break it again in D7, IMO. Edit: We can remove/repurpose the test in D8 as needed if/when we do the refactor.

Closed #1399364: hook_block_view_MODULE_DELTA_alter() doesn't always translate to a valid function as a duplicate of this issue.

Maybe is better if we enforce that block deltas must have underscores instead of hyphens (just like hook_node_info() do with machine names). Otherwise, things can be broken if some module do something like:

function mymodule_block_info() {
  return array(
    'delta-1' => array(
      'info' => 'Some block'
    ),
    'delta_1' => array(
      'info' => 'Other block'
    )
  );
}

@danillonunes I can see how modules that let the user inform a block delta might fall into that case. However, from reading http://api.drupal.org/api/drupal/modules%21node%21node.api.php/function/... I didn't see any enforcing of that rule. There's only a suggestion in the documentation.

#14 has a valid point. Now with implementing this change I agree that we can support more robust code and more edge cases, however I don't believe that we can support a valid change/fix if we go and create a new problem(s). We, as a community, start to become more like spaghetti in this fashion. A more complete fix would be to enforce the no hyphen rule, rebuild the menu module and have an upgrade path that includes fixing any remaining db instances of deltas with hyphens (which in core does not seem like there is many).

Yes the above entails involving more people but I think it is warranted. Thoughts? Open to discussion

If we are to implement danillonunes suggestion in #14, we better open a new issue and fix d8 (if it needs fixing). I don't think it's worth backporting to d7, since we can only provide upgrade paths to core.

In D8 it does appear as though menu_name is now fixed with no hyphen in the default configs (account, admin, footer, main, tools). The only thing that needs to be made sure of is that end users don't enter in a menu (machine) name that has hyphens, probably best accomplished using form validation as well as api validation.

In D7 however a few passes with the ctrl-f and find and replace ought to do the trick within just the menu module itself (menu.inc, menu.module, menu.admin.inc). Good practice here as well would be to upgrade current menu_custom db entries that have hyphens and do the same validation as above.

Both will need slight documentation tweaks, and some warning to admins when upgrading drupal.

I believe it should only be changed for D8 (machine name validation) and added to Drupal core upgrade path. As mentioned before, I don't think we should enforce this change for D7 since it might break contributed modules.

Thanks barraponto, I agree we ought to update the documentation about this known issue and let others know that in D8 this will no longer an issue.

Url for that page in the api is http://api.drupal.org/api/drupal/modules!block!block.api.php/function/ho...

Something along the lines of

"PLEASE NOTE: When using hook_block_view_MODULE_DELTA_alter for menus that you would like to alter, please keep in mind that in drupal 7 the block module will convert hyphens (-) into underscores (_) and that function names with hyphens will trigger a php fatal error. You can either go back and make your menu machine name without hyphens OR change your hook definition to account for this."

@coolestdude1 it's not as easy as it is in github, but I'm pretty sure you can patch that. Give it a try!

I will be leaving the version alone as the upstream changes matter more for right now. So this patch will probably fail tests because of that but here is the patch file against D7 Core for that documentation update.

My git bash glitched here is the correct file

My experience with D8 is very limited, and so much code has changed. I was able to track down the relavant part to core/modules/block/lib/Drupal/block/BlockRenderController.php, which looks like this:

  /**
   * Implements Drupal\Core\Entity\EntityRenderControllerInterface::viewMultiple().
   */
  public function viewMultiple(array $entities = array(), $view_mode = 'full', $langcode = NULL) {
    $build = array();
    foreach ($entities as $entity_id => $entity) {
      // Allow blocks to be empty, do not add in the defaults.
      if ($content = $entity->getPlugin()->build()) {
        $build[$entity_id] = $this->getBuildDefaults($entity, $view_mode, $langcode);
      }
      $build[$entity_id]['content'] = $content;

      // All blocks, even when empty, should be available for altering.
      $id = str_replace(':', '__', $entity->get('plugin'));
      list(, $name) = $entity->id();
      drupal_alter(array('block_view', "block_view_$id", "block_view_$name"), $build[$entity_id], $entity);

    }
    return $build;
  }

You can see that some string replacement is already being applied.

My concern right now is that, while this issue sits in the queue, a real bug is going unfixed for a long time. If we can't get someone who knows D8 well enough to know that no more string replacements are needed, then we should switch the focus to D7 before it's too late.

#4 is exactly what we need to fix this bug. The patch has already been tested by several people, including me. I think the only thing missing is a test case.

I'm closing my own duplicate, #1821004: Alter delta before executing hook_block_view_MODULE_DELTA_alter.

Here is a patch with tests.

@fizk - Thanks for the patch, we do need to fix this in D8 first though, see #10.

Tests only patch didn't contain the actual test.

@Cottser, thanks, but no one has established that this is a bug in D8. If someone can show this, then we can set this back to 8.x.

@fizk - If you could forward port your test to 8.x that would likely show us whether the bug still exists in 8.x or not. Thanks for your continued work on this issue!

Fixing version again. Thanks! Our backport policy indicates that we need to confirm whether this exists in D8 or not, and add coverage for it and fix it if appropriate, before we add the change to D7.

Forgot to update, it does look like D8 is affected by this - hook_block_view_ID_alter() and hook_block_view_NAME_alter() are not called when a hyphen is present.

hook_block_view_NAME_alter() is actually broken in another way as well:

...
list(, $name) = $entity->id();
drupal_alter(array('block_view', "block_view_$id", "block_view_$name"), $build[$entity_id], $entity);

As far as I can tell, $entity->id() never returns an array, just a string. That probably should be changed to:

$name = $entity->id();

I'll try to post D8 tests.

Here is a D8 fix and test.

@fizk - Thank you, that's great! Next time, if you upload your test-only patch first (red then green), we won't get the automatic "needs work" (#34).

Thanks @fizk; that's an excellent test, and I'm glad you caught the bug in D8 too. In fact, this sort of thing is exactly why we have the backport policy--we broke it again in D8, differently, without realizing it. :) Overall I think this patch looks good; I just have a few polishes to add to the test so that it's easier for people to understand what it's doing in the future.

1. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockViewAlterTest.phpundefined
   @@ -0,0 +1,51 @@
   + * Definition of Drupal\block\Tests\BlockViewAlterTest.
   

   So the standard for this changed last summer. It should be:

   Contains \Drupal\block\Tests\BlockViewAlterTest.
   

2. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockViewAlterTest.phpundefined
   @@ -0,0 +1,51 @@
   +    // Enable our test blocks.
   +    $this->drupalPlaceBlock('test-id-hyphen', array('machine_name' => 'test-name-hyphen'));
   

   I'd move this to the beginning of the test method itself. At first I was really confused as to how the test method was providing coverage for the bug.

3. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockViewAlterTest.phpundefined
   @@ -0,0 +1,51 @@
   +   * Tests if the view alter function is called when the block id or
   +   * machine name contains a hyphen.
   

   The docblock should be a single line of 80 characters or less. Also, note that "ID" should always be capitalized.

4. +++ b/core/modules/block/lib/Drupal/block/Tests/BlockViewAlterTest.phpundefined
   @@ -0,0 +1,51 @@
   +    $this->assertText('hook_block_view_ID_alter', 'Block content displays.');
   +    $this->assertText('hook_block_view_NAME_alter', 'Block content displays.');
   

   Let's remove the assertion messages here -- they provide less information than the default assertion message will in this case.

   Also, an inline comment referencing block_test_block_view_test_id_hyphen_alter() would help here, so that folks in the future know what this is actually testing (that the alter hooks are actually invoked). Edit: actually, instead of an inline comment, see the next point.

5. +++ b/core/modules/block/tests/block_test.moduleundefined
   @@ -12,3 +12,17 @@ function block_test_system_theme_info() {
   +    $build['content']['#children'] .= 'hook_block_view_ID_alter';
   ...
   +    $build['content']['#children'] .= 'hook_block_view_NAME_alter';
   

   Hook and function names should always have parens when used in text. Also, let's make it a little clearer what the purpose is here, i.e.:
   hook_block_view_ID_alter() invoked.

With those simple cleanups I think this patch will be ready. Also tagging as a block plugins followup because we apparently introduced this bug in a brand new way with that conversion...

@Cottser, Thanks for the tip!

@xjm, Here's the patch with your suggested improvements.

Fixing tags.

needs backport to D7 - this needs to be fixed in D8 first.
Needs tests - we have tests.
Novice - no longer novice IMHO :)

@fizk, thanks for the revised patch! Interdiffs are also very helpful for reviewers and therefore moving issues along quicker :)

If you'd like to get this fix into D7, it's in your best interest to keep the backport tag intact. When a core committer sees the backport tag, after a commit they will set the issue to "patch (to be ported)" and change the issue version back to the relevant version (in this case 7.x-dev). At that point you or someone else can post a 7.x patch to be reviewed. Without the tag, usually the status will go to "fixed" and the issue is automatically closed after 2 weeks of inactivity. I'm fine with the other two tag removals for the time being, although the backport itself is still probably a novice task.

+++ b/core/modules/block/lib/Drupal/block/Tests/BlockViewAlterTest.phpundefined
@@ -0,0 +1,51 @@
+ * Contains Drupal\block\Tests\BlockViewAlterTest.

As @xjm mentioned, this needs to be "Contains \Drupal…", just missing the leading slash.

Also, for point #3 in #36, ID was capitalized but the description should be shortened to fit on a line of less than 80 characters. Ref: http://drupal.org/node/1354#drupal - "All summaries (first lines of docblocks)…"

NW and novice for those last couple minor cleanups (per #39). Thanks!

friesk via core.drupalofficehours and #drupal ksf

@friesk, I don't understand your changes, and what "friesk via core.drupalofficehours and #drupal ksf" means?

@fizk, @friesk is currently working on the cleanups suggested in #39, as a part of core mentoring. @friesk's username in IRC is ksf.

Please don't unassign issues from other people so quickly without asking first, since d.o does not allow reassigning to arbitrary users. :)

Assigning to myself until friesk can take it back.

Ah, ok, sorry about that. I don't think this should have been marked as task, though, nor made active instead of needs work.

Ah, ok, sorry about that. I don't think this should have been marked as task, though, nor made active instead of needs work.

Aye, agreed. :)

my bad, still learning the vocab

Endless mentoring props to xjm.

interdiff

new patch

This looks good to me!

@xjm, can you set this to RTBC?

Thanks @fizk and @friesk!

This clashes with the direction taken in #1927608: Remove the tight coupling between Block Plugins and Block Entities. Not unRTBCing yet because this seems to have been a pre-existing bug, but I'd be grateful if this was postponed.

This is postponed on #1927608: Remove the tight coupling between Block Plugins and Block Entities...

Also I think the patch can be improved... see interdiff attached

This is moot in D8, hook_block_view_MODULE_DELTA_alter() was removed.

Thanks Tim, for people trying to port their modules with hook_block_view_MODULE_DELTA_alter(), how would you accomplish this in D8?

Setting to RTBC from #54.

The patch in #54 was for D8 ... the last patch for D7 was #25

#25: 1076132-hook_block_view_MODULE_DELTA_alter-1.patch queued for re-testing.

@adaddinsane Looks like there's only hook_block_view_alter() and hook_block_view_BASE_BLOCK_ID_alter() in D8 now.

http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

The BASE_BLOCK_ID comes from BlockBase::getPluginId(). I'm not 100% sure if that function always returns strings that can be used directly in a callable function name (i.e. only letters, numbers, and underscores).

A block plugin ID must be machine_name safe, so a-zA-Z0-9_

Thanks for confirming Tim.

Setting to RTBC from #54.

+++ b/modules/block/block.testundefined
@@ -753,6 +753,45 @@ class BlockTemplateSuggestionsUnitTest extends DrupalUnitTestCase {
+ * Unit tests for hook_block_view_MODULE_DELTA_alter().
...
+class BlockViewModuleDeltaAlterUnitTest extends DrupalWebTestCase {

This is not actually a unit test, it's a web test.

+++ b/modules/block/block.testundefined
@@ -753,6 +753,45 @@ class BlockTemplateSuggestionsUnitTest extends DrupalUnitTestCase {
+class BlockViewModuleDeltaAlterUnitTest extends DrupalWebTestCase {
+  public static function getInfo() {
...
+    $this->assertEqual($test_hyphen, 'hook_block_view_MODULE_DELTA_alter', 'Hyphens (-) in block delta were replaced by underscore (_)');
+  }
+}

Missing a blank line after beginning and before the end of the class.

+++ b/modules/block/block.testundefined
@@ -753,6 +753,45 @@ class BlockTemplateSuggestionsUnitTest extends DrupalUnitTestCase {
+  function setUp() {
...
+  function testBlockViewModuleDeltaAlter() {

These should be marked public

+++ b/modules/block/tests/block_test.moduleundefined
@@ -34,3 +42,17 @@ function block_test_block_info() {
+    $data['content'] = 'hook_block_view_MODULE_DELTA_alter';
...
+    $data['content'] = 'hook_block_view_MODULE_DELTA_alter';

These are indented 2 spaces too many

Quick fixes though! Almsot done.

I'll fix these small things.

@Sethie - @Cottser suggested I take a shot at these fixes as part of core mentoring. If you're busy with it and want to complete it just let me know and I'll find something else :-)

Not 100% sure I got all the suggested changes but here's a first attempt. My interdiff doesn't look right though - I thought it was supposed to just reflect the changes I made after the previous patch...

I also wasn't quite sure what the difference between the 2 files: hook_block_view_MODULE_DELTA_alter-64.patch and hook_block_view_MODULE_DELTA_alter-64-tests-only.patch that were attached to comment 64. Was I supposed to work on both? I only worked on the file: hook_block_view_MODULE_DELTA_alter-64.patch

Thanks @foxtrotcharlie!

+++ b/modules/block/block.test
@@ -897,2 +899,3 @@
+  ¶

We got some extra trailing whitespace here that needs to be removed per http://drupal.org/coding-standards#indenting.

Thank you @Cottser. I have now set up my IDE to remove trailing whitespace. Patch with whitespace removed, attached.

Marking RTBC.

+++ b/modules/block/block.test
@@ -193,7 +193,7 @@ class BlockTestCase extends DrupalWebTestCase {
+   * Test block visibility when using "pages" restriction but leaving

This formatting change is unrelated to the fix. As per the coding standards, this needs to be in a separate issue.

Note: Do not squeeze coding standards updates/clean-ups into otherwise unrelated patches. Only touch code lines that are actually relevant. To update existing code for the current standards, always create separate and dedicated issues and patches.

Here is the rerolled version.

I think we're getting a little bit too picky with our changes now. Can we RTBC this, commit it to HEAD, and then make further improvements if needed?

tim.plunkett's review in #67 wasn't fully addressed, plus calling array_pop() directly on a function result produces E_STRICT errors...

Fixed those in the attached, plus a small code style issue (function summary should be on one line).

These are small fixes, so I will commit it as long as it passes tests.

Committed to 7.x - thanks! http://drupalcode.org/project/drupal.git/commit/226fe69