Block example's hook_block_view_alter() needs work [#1296452]

Problem/Motivation

The hook_block_view_alter() function in the Block Example doesn't work as written. The problem is that it eliminates all content that is not a string.

if (!isset($data['content']) || !is_string($data['content'])) {
    return;
  }

But the configurable block created by the module returns the content in $data['content']['#markup'], an array.
Also, blocks created in the UI return the title in $block->title. As written, the only blocks it will affect are blocks created in code that return a string for $data['content']

Proposed resolution

That leaves three possibilities:

Change the code to work for the configurable block created by the module.
Change the code to work for UI created blocks or other blocks created programmatically that return a string.
Change the code to accommodate both.

Remaining tasks

The original poster tried to write a solution to fit #1. He says it doesn't work. Possibly because he targeted the default title then changed it in the UI.
The patch in comment #3 offers a solution for #2 above but can be ignored.
The patch in #7 solves #3 and needs to be reviewed. Tests need to be revised.

User interface changes

n/a

API changes

n/a

Original report by dashohoxha

First of all, I found out that it had some bugs and corrected it like this:

/**
 * Implements hook_block_view_alter().
 *
 * This hook allows you to modify the output of any block in the system.
 *
 * In addition, instead of hook_block_view_alter(), which is called for all
 * blocks, you can also use hook_block_view_MODULE_DELTA_alter() to alter a
 * specific block.
 *
 * We are going to uppercase the title of any block if the string "magic string"
 * is encountered in the content. If we were changing only our block using
 * hook_block_view_MODULE_DELTA_alter to do this, we would have used the
 * function:
 * block_example_block_view_block_example_example_configurable_text_alter()
 *
 * To demonstrate the effect of this hook, you can use the
 * 'configurable_text_string' block created by this module and add the
 * text 'magic string' into the configuration.
 */
function block_example_block_view_alter(&$data, $block) {
  if ($block->module != 'block_example') { return; }
  debug($data, 'data', true);

  // If the content contains the string: 'magic string', uppercase the title.
  if (strstr($data['content']['#markup'], 'magic string')) {
    $data['subject'] = isset($data['subject']) ? drupal_strtoupper($data['subject']) : '';
    debug($data['subject'], 'data_subject');
  }
}

However it still does not work. The subject seems to have the default value, which is later replaced by the custom value. I don't know how to fix this.

Comment	File	Size	Author
#23	hook_block_view_alter-1296452-20.patch	6.44 KB	jn2
#23
#20	hook_block_view_alter-1296452-20.patch	6.44 KB	jn2
#20
#16	hook_block_view_alter-1296452-14.patch	0 bytes	jn2
#16
#14	hook_block_view_alter-1296452-14.patch	664.07 KB	jn2
#14
#13	hook_block_view_alter-1296452-13.patch	6.28 KB	jn2
#13
#12	examples.block_example_hook_block_view_alter.patch	6.04 KB	rfay
#12
#9	fixed_hook_block_view_alter-1296452-9.patch	4.18 KB	jn2
#9
#7	fixed_hook_block_view_alter-1296452-7.patch	6.02 KB	jn2
#7
#3	fixed_hook_block_view_alter-1296452-3.patch	2.55 KB	jn2
#3

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 1 October 2011 at 17:03

Status:

Active

» Postponed (maintainer needs more info)

Hi - Thanks for your report.

Please, though, explain *what* bugs. That's the first thing in an issue. Explain what you expected and what happened instead.

Then, since you've got the ability to fix it, please provide a patch that fixes the problem you've discovered.

You can set this to "active" or "needs review" (if you've provided a patch) when you respond. Please feel free to edit the node itself and put a good issue summary there.

Thanks,
-Randy

Comment #2

jn2 CreditAttribution: jn2 commented 4 October 2011 at 16:03

I've been meaning to look into this for awhile.

As I see it, the problem is that strstr only works on a string, and the configurable block returns an array. So this 'magic string' exercise won't work on the block created by the module.

It does work on another block I tried it with. This one was created in code by a different module and has the title in $data['subject'].

It does not work on a block created in the UI. Those blocks place the title in $block->title, not in $data['subject']. I'll be back soon with a possible patch.

Comment #3

jn2 CreditAttribution: jn2 commented 4 October 2011 at 20:30

Title:

Block example's hook_block_view_alter() needs work

» hook_block_view_alter() does not work

File	Size
fixed_hook_block_view_alter-1296452-3.patch	2.55 KB

Here's an attempt to fix this. Open to better ideas.

Comment #4

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 4 October 2011 at 18:05

Status:

Postponed (maintainer needs more info)

» Needs review

Thanks so much! Could you update the issue summary and the title please?

Comment #4.0

jn2 CreditAttribution: jn2 commented 4 October 2011 at 18:44

Issue summary:

View changes

Updating with better understanding of the problem and new patch.

Comment #4.1

jn2 CreditAttribution: jn2 commented 4 October 2011 at 18:55

Issue summary:

View changes

minor edit

Comment #5

jn2 CreditAttribution: jn2 commented 4 October 2011 at 19:19

Title:

hook_block_view_alter() does not work

» Block example's hook_block_view_alter() needs work

@rfay
Do you know why the test status is 'postponed'? Don't think I've seen that before.

Comment #6

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 4 October 2011 at 19:34

It's postponed because the branch test is broken. http://qa.drupal.org/pifr/test/136969

I fixed that in D6; I guess I didn't realize it was also broken in D7.

Comment #7

jn2 CreditAttribution: jn2 commented 5 October 2011 at 18:43

Title:

hook_block_view_alter() does not work

» Block example's hook_block_view_alter() needs work

File	Size
fixed_hook_block_view_alter-1296452-7.patch	6.02 KB

I did more work on the patch, and now it works for both the configurable text block created by the module and any block created in the UI. It may be a bit confusing to read since there are two commits. I'll have to learn how to squash commits in Git.

Also, I ran the block tests, and this breaks the hook_block_view_alter test. (Passed the test when it didn't work, now breaks the test when it does.) I don't have much experience with tests, but I'll try to look at it.

Comment #7.0

jn2 CreditAttribution: jn2 commented 5 October 2011 at 18:43

Issue summary:

View changes

minor edit

Comment #8

15 October 2011 at 06:15

Status:

Needs review

» Needs work

The last submitted patch, fixed_hook_block_view_alter-1296452-7.patch, failed testing.

Comment #9

jn2 CreditAttribution: jn2 commented 15 October 2011 at 22:16

Status:

Needs work

» Needs review

File	Size
fixed_hook_block_view_alter-1296452-9.patch	4.18 KB

This one should work. Test is fixed.

Comment #10

jn2 CreditAttribution: jn2 commented 15 October 2011 at 22:18

This one should work. Test is fixed.

Hmmm. Don't know why this saved twice.

Comment #11

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 15 October 2011 at 23:35

Status:

Needs review

» Needs work

Thanks for all your effort on this!

This seems too complex to me. I worked on it a bit on the plane yesterday but didn't quite get it done. The idea is use drupal_render() (on a copy of $data) to render the $data and look for the string in it. Becomes much simpler.

It's OK with me to completely change the driver. In other words, we're trying to demonstrate something. It *doesn't* have to be "looking for a string in the content". We're just using an arbitrary thing.

Let's either render or use a different approach. The fact that I did some random thing in D6 doesn't mean we have to stick with that demonstration. We just want to do an alter that people can understand :-)

Comment #12

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 18 October 2011 at 21:24

Status:

Needs work

» Needs review

File	Size
examples.block_example_hook_block_view_alter.patch	6.04 KB

Here's a new version for your review. This one provides another sample block (to be capitalized) and triggers off of the block title instead of other stuff.

Comment #13

jn2 CreditAttribution: jn2 commented 26 November 2011 at 15:10

File	Size
hook_block_view_alter-1296452-13.patch	6.28 KB

This demonstration is much cleaner. I tried it out, created a block with the word 'uppercase' in the title, ran the tests. Everything works fine.

I tweaked the patch a bit, but you could go with #12 as is. There was one typo in the 'uppercase this please' block content (first sentence not capitalized); you might want to fix that.

I also expanded a little on the subject vs title keys. Might be overkill so could be omitted.

Comment #14

jn2 CreditAttribution: jn2 commented 19 February 2012 at 21:24

File	Size
hook_block_view_alter-1296452-14.patch	664.07 KB

Here's another patch with slight clarifications, no code changes.

Comment #15

19 February 2012 at 21:26

Status:

Needs review

» Needs work

The last submitted patch, hook_block_view_alter-1296452-14.patch, failed testing.

Comment #16

jn2 CreditAttribution: jn2 commented 19 February 2012 at 21:32

Status:

Needs work

» Needs review

File	Size
hook_block_view_alter-1296452-14.patch	0 bytes

Oops. This one should work.

Comment #17

jn2 CreditAttribution: jn2 commented 19 February 2012 at 21:55

This is fixed for 7.x-1.x. Need to test it on D8. Edited: that patch didn't make right either.

Comment #18

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 19 February 2012 at 21:43

Version:

7.x-1.x-dev

The one that "should work", #16 was empty...

Comment #19

jn2 CreditAttribution: jn2 commented 19 February 2012 at 21:56

Yeah, I just noticed that.

Comment #20

jn2 CreditAttribution: jn2 commented 19 February 2012 at 22:15

File	Size
hook_block_view_alter-1296452-20.patch	6.44 KB

Trying again. At least this one's not empty. :)

Comment #21

jn2 CreditAttribution: jn2 commented 19 February 2012 at 23:09

Version:

» 7.x-1.x-dev

Needs to test again 7.x-1.x. (Testing postponed for some reason.)

Comment #22

rfay

English

Palisade, CO, USA

CreditAttribution: rfay commented 19 February 2012 at 23:15

8.x-1.x has been broken for some months. That's why it's postponed.

Comment #23

jn2 CreditAttribution: jn2 commented 19 February 2012 at 23:26

File	Size
hook_block_view_alter-1296452-20.patch	6.44 KB

Okay, then this should test for 7.x-1.x.

Comment #24

19 February 2012 at 23:28

Status:

Needs review

» Needs work

The last submitted patch, hook_block_view_alter-1296452-20.patch, failed testing.

Comment #25

jn2 CreditAttribution: jn2 commented 19 February 2012 at 23:30

Status:

Needs work

» Needs review

#13: hook_block_view_alter-1296452-13.patch queued for re-testing.

Comment #26

jn2 CreditAttribution: jn2 commented 20 February 2012 at 00:02

@rfay

Here's what happened. My FAPI reference habits got to me. There I don't make patches, I just push. So I made one small change to the patch in #13 and pushed before realizing I needed a patch for the issue.

#14 was a mistake made against Master, then #16 of course is empty because once the changes are pushed, there's no diff. Now #13 doesn't apply because the small change I made conflicts with it. #20 was an attempt to create a patch with the change, but that experiment obviously didn't work either.

Unless you want to revert the commit, this does remain fixed for 7.x-1.x. Tests on the local install run fine.

What happens with 8.x-1.x? Is it abandoned? The patch in #20 applied cleanly to 8.x-1.x. It works as it should, but testing is broken.