Simple problem: menu block deltas are "menu-xxxx", they use hyphens instead of underscores but the block module fails to change the hyphens to underscores when calling drupal_alter with "block_view_MODULE_DELTA".

Eclipse+Git doesn't let me create patches (grrrrr) so here's my fix:

        // Allow modules to modify the block before it is viewed, via either
        // hook_block_view_alter() or hook_block_view_MODULE_DELTA_alter().
        $delta = str_replace('-', '_', $block->delta);
        drupal_alter(array('block_view', "block_view_{$block->module}_{$delta}"), $array, $block);

Hope that helps.

Files: 
CommentFileSizeAuthor
#79 interdiff-77-79.txt1.82 KBDavid_Rothstein
#79 d7-menu-block-delta-1076132-79.patch3.85 KBDavid_Rothstein
PASSED: [[SimpleTest]]: [MySQL] 40,478 pass(es). View
#77 interdiff-1076132-74-77.txt465 bytesskwashd
#77 d7-menu-block-delta-1076132-77.patch4.41 KBskwashd
PASSED: [[SimpleTest]]: [MySQL] 40,505 pass(es). View
#74 interdiff.txt416 bytesfoxtrotcharlie
#74 d7-menu-block-delta-1076132-74.patch4.11 KBfoxtrotcharlie
PASSED: [[SimpleTest]]: [MySQL] 40,445 pass(es). View
#71 interdiff.txt3.29 KBfoxtrotcharlie
#71 d7-menu-block-delta-1076132-71.patch4.1 KBfoxtrotcharlie
PASSED: [[SimpleTest]]: [MySQL] 40,245 pass(es). View
#64 1076132-hook_block_view_MODULE_DELTA_alter-64-tests-only.patch2.92 KBfizk
FAILED: [[SimpleTest]]: [MySQL] 40,095 pass(es), 1 fail(s), and 1 exception(s). View
#64 1076132-hook_block_view_MODULE_DELTA_alter-64.patch3.8 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 40,313 pass(es). View
#56 51-55-interdiff.txt2.05 KBalexpott
#56 block-1076132-55.patch6.78 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1076132-55.patch. Unable to apply patch. See the log in the details link for more information. View
#51 block-1076132-51.patch6.08 KBfriesk
PASSED: [[SimpleTest]]: [MySQL] 53,423 pass(es). View
#49 interdiff.txt1.1 KBfriesk
#48 block-1076132-48.patch9.14 KBfriesk
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#48 1076132-hook_block_view_MODULE_DELTA_alter-4.patch5.61 KBfriesk
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
#37 1076132-hook_block_view_MODULE_DELTA_alter-4-tests-only.patch3.34 KBfizk
FAILED: [[SimpleTest]]: [MySQL] 53,213 pass(es), 2 fail(s), and 0 exception(s). View
#37 1076132-hook_block_view_MODULE_DELTA_alter-4.patch5.61 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 53,216 pass(es). View
#33 1076132-hook_block_view_MODULE_DELTA_alter-3.patch5.62 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 53,167 pass(es). View
#33 1076132-hook_block_view_MODULE_DELTA_alter-3-tests-only.patch3.36 KBfizk
FAILED: [[SimpleTest]]: [MySQL] 53,134 pass(es), 2 fail(s), and 0 exception(s). View
#27 1076132-hook_block_view_MODULE_DELTA_alter-2-tests-only.patch2.92 KBfizk
FAILED: [[SimpleTest]]: [MySQL] 40,289 pass(es), 1 fail(s), and 1 exception(s). View
#25 1076132-hook_block_view_MODULE_DELTA_alter-1.patch3.8 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 40,368 pass(es). View
#25 1076132-hook_block_view_MODULE_DELTA_alter-1-tests-only.patch1.16 KBfizk
PASSED: [[SimpleTest]]: [MySQL] 40,094 pass(es). View
#23 block_api_documentation-1076132-23.patch946 bytescoolestdude1
PASSED: [[SimpleTest]]: [MySQL] 40,295 pass(es). View
#22 block_api_documentation-1076132-22.patch0 bytescoolestdude1
PASSED: [[SimpleTest]]: [MySQL] 40,312 pass(es). View
#4 block_menu_hooks.patch810 byteststoeckler
PASSED: [[SimpleTest]]: [MySQL] 36,058 pass(es). View

Comments

dcrocks’s picture

wojtha’s picture

Status: Active » Closed (duplicate)
tstoeckler’s picture

Status: Closed (duplicate) » Active

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.

tstoeckler’s picture

Version: 7.0-rc4 » 7.x-dev
Status: Active » Needs review
FileSize
810 bytes
PASSED: [[SimpleTest]]: [MySQL] 36,058 pass(es). View

Here we go.

tstoeckler’s picture

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.

dozymoe’s picture

0.0

tstoeckler’s picture

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.

selltrends’s picture

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

barraponto’s picture

Status: Needs review » Reviewed & tested by the community

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.

xjm’s picture

Version: 7.x-dev » 8.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests, +needs backport to D7

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.

tstoeckler’s picture

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.

xjm’s picture

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.

Cottser’s picture

danillonunes’s picture

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'
    )
  );
}
barraponto’s picture

@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.

coolestdude1’s picture

#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

barraponto’s picture

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.

coolestdude1’s picture

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.

barraponto’s picture

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.

coolestdude1’s picture

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."

barraponto’s picture

Issue tags: +Novice

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

coolestdude1’s picture

FileSize
0 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,312 pass(es). View

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.

coolestdude1’s picture

FileSize
946 bytes
PASSED: [[SimpleTest]]: [MySQL] 40,295 pass(es). View

My git bash glitched here is the correct file

fizk’s picture

Version: 8.x-dev » 7.x-dev

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.

fizk’s picture

Status: Needs work » Needs review
FileSize
1.16 KB
PASSED: [[SimpleTest]]: [MySQL] 40,094 pass(es). View
3.8 KB
PASSED: [[SimpleTest]]: [MySQL] 40,368 pass(es). View

Here is a patch with tests.

Cottser’s picture

Version: 7.x-dev » 8.x-dev

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

fizk’s picture

Version: 8.x-dev » 7.x-dev
FileSize
2.92 KB
FAILED: [[SimpleTest]]: [MySQL] 40,289 pass(es), 1 fail(s), and 1 exception(s). View

Tests only patch didn't contain the actual test.

fizk’s picture

@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.

Status: Needs review » Needs work

The last submitted patch, 1076132-hook_block_view_MODULE_DELTA_alter-2-tests-only.patch, failed testing.

Cottser’s picture

@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!

xjm’s picture

Version: 7.x-dev » 8.x-dev

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.

fizk’s picture

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.

fizk’s picture

Status: Needs work » Needs review
FileSize
3.36 KB
FAILED: [[SimpleTest]]: [MySQL] 53,134 pass(es), 2 fail(s), and 0 exception(s). View
5.62 KB
PASSED: [[SimpleTest]]: [MySQL] 53,167 pass(es). View

Here is a D8 fix and test.

Status: Needs review » Needs work

The last submitted patch, 1076132-hook_block_view_MODULE_DELTA_alter-3-tests-only.patch, failed testing.

Cottser’s picture

Status: Needs work » Needs review

@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).

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Block plugins

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...

fizk’s picture

Status: Needs work » Needs review
FileSize
5.61 KB
PASSED: [[SimpleTest]]: [MySQL] 53,216 pass(es). View
3.34 KB
FAILED: [[SimpleTest]]: [MySQL] 53,213 pass(es), 2 fail(s), and 0 exception(s). View

@Cottser, Thanks for the tip!

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

fizk’s picture

Fixing tags.

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

Cottser’s picture

Issue tags: +needs backport to D7

@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)…"

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Novice

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

friesk’s picture

Assigned: Unassigned » friesk
Category: bug » task
Status: Needs work » Active

friesk via core.drupalofficehours and #drupal ksf

fizk’s picture

Assigned: friesk » Unassigned
Category: task » bug
Status: Active » Needs work

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

xjm’s picture

@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. :)

xjm’s picture

Assigned: Unassigned » xjm

Assigning to myself until friesk can take it back.

fizk’s picture

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

xjm’s picture

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. :)

friesk’s picture

my bad, still learning the vocab

friesk’s picture

Assigned: xjm » friesk
Status: Needs work » Needs review
FileSize
5.61 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View
9.14 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion. View

Endless mentoring props to xjm.

friesk’s picture

FileSize
1.1 KB

interdiff

Status: Needs review » Needs work

The last submitted patch, 1076132-hook_block_view_MODULE_DELTA_alter-4.patch, failed testing.

friesk’s picture

Status: Needs work » Needs review
FileSize
6.08 KB
PASSED: [[SimpleTest]]: [MySQL] 53,423 pass(es). View

new patch

fizk’s picture

Status: Needs review » Reviewed & tested by the community

This looks good to me!

@xjm, can you set this to RTBC?

fizk’s picture

Status: Reviewed & tested by the community » Needs review
xjm’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @fizk and @friesk!

tim.plunkett’s picture

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.

alexpott’s picture

Status: Reviewed & tested by the community » Postponed
FileSize
6.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch block-1076132-55.patch. Unable to apply patch. See the log in the details link for more information. View
2.05 KB

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

fizk’s picture

Assigned: friesk » Unassigned
tim.plunkett’s picture

Version: 8.x-dev » 7.x-dev
Status: Postponed » Needs work

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

fizk’s picture

Status: Needs work » Reviewed & tested by the community

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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, block-1076132-55.patch, failed testing.

adaddinsane’s picture

In D8 blocks are entities so respond to all the entity hooks and specifically you have:

hook_block_view_alter(array &$build, \Drupal\block\Plugin\Core\Entity\Block $block)

and even

hook_block_view_ID_alter(array &$build, \Drupal\block\Plugin\Core\Entity\Block $block)
hook_block_view_NAME_alter(array &$build, \Drupal\block\Plugin\Core\Entity\Block $block)

bleen’s picture

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

bleen’s picture

Status: Needs work » Needs review
fizk’s picture

FileSize
3.8 KB
PASSED: [[SimpleTest]]: [MySQL] 40,313 pass(es). View
2.92 KB
FAILED: [[SimpleTest]]: [MySQL] 40,095 pass(es), 1 fail(s), and 1 exception(s). View

@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).

tim.plunkett’s picture

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

fizk’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: +needs backport to D7, +Block plugins

Thanks for confirming Tim.

Setting to RTBC from #54.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community
+++ 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

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -needs backport to D7, -Block plugins

Quick fixes though! Almsot done.

Sethie’s picture

Assigned: Unassigned » Sethie
Status: Reviewed & tested by the community » Needs work
Issue tags: -needs backport to D7, -Block plugins

I'll fix these small things.

foxtrotcharlie’s picture

Assigned: Sethie » foxtrotcharlie

@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 :-)

foxtrotcharlie’s picture

Issue summary: View changes
FileSize
4.1 KB
PASSED: [[SimpleTest]]: [MySQL] 40,245 pass(es). View
3.29 KB

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

foxtrotcharlie’s picture

Status: Needs work » Needs review
Cottser’s picture

Status: Needs review » Needs work

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.

foxtrotcharlie’s picture

Status: Needs work » Needs review
FileSize
4.11 KB
PASSED: [[SimpleTest]]: [MySQL] 40,445 pass(es). View
416 bytes

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

fizk’s picture

Status: Needs review » Reviewed & tested by the community

Marking RTBC.

skwashd’s picture

Status: Reviewed & tested by the community » Needs work
+++ 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.

skwashd’s picture

Status: Needs work » Needs review
FileSize
4.41 KB
PASSED: [[SimpleTest]]: [MySQL] 40,505 pass(es). View
465 bytes

Here is the rerolled version.

fizk’s picture

Status: Needs review » Reviewed & tested by the community

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?

David_Rothstein’s picture

FileSize
3.85 KB
PASSED: [[SimpleTest]]: [MySQL] 40,478 pass(es). View
1.82 KB

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.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: +7.25 release notes

Status: Fixed » Closed (fixed)

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