Since we added custom titles in http://cvs.drupal.org/viewcvs/drupal/drupal/modules/block/block.module?r... a called to module_invoke('block', 'view', ...) (like from panels.module) doesn't get us the subject. Only in block_list() do we determine the subject.
This patch does result in a double check_plain since block_list() does a check plain on subject as well. There may be more cleanup needed between block_list and block_block('view').
Also includes a proper initialization of $data.
Comment | File | Size | Author |
---|---|---|---|
#52 | 126070-52-block-view-empty-subject.patch | 2.54 KB | JohnAlbin |
#50 | block-block-view-126070-50.patch | 4.37 KB | David_Rothstein |
#45 | drupal.block-block-view.45.patch | 3 KB | sun |
#42 | 126070-block_block_view-d.patch | 2.91 KB | manarth |
#40 | 126070-block_block_view-c.patch | 2.6 KB | manarth |
Comments
Comment #1
zostay CreditAttribution: zostay commentedThis problem has caused a bug in the Panels plugin which isn't able to show the title correctly for custom blocks. The Panels plugin could work around the issue, but it really ought to be, IMO, fixed in the next release.
Thanks for the patch.
+1 fwiw
Comment #2
RobRoy CreditAttribution: RobRoy commentedMoving to HEAD.
Comment #3
bdragon CreditAttribution: bdragon commentedFree reroll courtesy patch bingo.
Comment #4
birdmanx35 CreditAttribution: birdmanx35 commentedJust retested, and this patch still works, although I don't know much more than that (I'm a patch newbie, just punching the bingo button).
Comment #5
sethcohn CreditAttribution: sethcohn commentedHappened on this in 5.x, when doing block adding the 'right way' (per numerous docs out there) using 'module_invoke' on the block module itself, and despite the documentation, it was NOT returning the 'subject' part of the array, only the content part.
The workaround answer is: do NOT use the module_invoke:
$data = module_invoke ('block', 'block', 'view', $delta)
and instead do this:
A bit frustrating this patch never made it into 5.x, since it's clearly NOT working per the API documentation that says it should. This is a serious bug, not a feature, and it should work in 5.x:
http://api.drupal.org/api/function/hook_block/5
yet, the block module itself fails to meet this spec for the hook.
Comment #6
drummThe patch is for Drupal 6.x and should be reviewed/committed there first.
Comment #7
Gábor HojtsyThere is an admitted double check_plain() happening from this patch, so although it looks good in principle, this is not to be committed as-is. hook_block() is not documented to expect a check_plain()-ed return value on the subject field, so this might be safe to remove. I wonder why RobRoy thought it is important to keep check_plain() here as well?
Comment #8
catchBumping to D7.
Comment #9
RobRoy CreditAttribution: RobRoy commentedI'm not sure why I put that in there. It can probably be safely removed.
Comment #10
pwolanin CreditAttribution: pwolanin commentedLooks like the D6/D7 patch needs to be a little different, since block subject is optional. #3 is probably fine
here is the relevant code for block_list():
from D6/D7 block_list():
note that you will NOT get check_plain() twice, since the patch returns $block->subject, and $block->title is not returned. block_list() will generate $block->subject again, but that's not a big deal.
We may be able to do some cleanup in coordination with this patch: http://drupal.org/node/232037
Comment #11
Robin Monks CreditAttribution: Robin Monks commentedPatch seems to work, block simpletest passes "58 passes, 0 fails, 0 exceptions". RTBC.
Robin
Comment #12
webchickDoesn't look like Gabor's comments in #7 were addressed.
Comment #13
shaneforsythe CreditAttribution: shaneforsythe commentedNot sure about the Original issue as seemed specific to Version 7.0 , this is against the current 6.8 download. The original issue mentioned something about "custom titles" which I suspect may have been back ported and perhaps broke functionality.
Going by the 6.X API here http://api.drupal.org/api/function/hook_block which states
This functionality is used/assumed in many of the phpsnippets given on how to generate block via code (see example below).
....
Additionally, as I delved into it I had questions about the design decision of the table structure and normalization.
Table boxes has the following columns
bid
body
info -> This is 'Block Description' in the admin section and what is used in list of modules
format
Table blocks stores the title -> this is the 'title' that is used when the box/block is rendered, and what is refered to in API as the return element 'subject'
As far as I can tell , if you configure a block under a specific theme, the title of all the blocks (one for each theme) is changed. So why isn't title just stored in table boxes?
Attached is the modification I made so that something similar can work
I am brand new to drupal and still trying to get a grasp of things , if I am misunderstanding some design spec I apologize but just trying to make sence of the API and examples given, and just documenting my discovery in case it helps anyone else follow the logic.
Comment #14
Gábor Hojtsy#355103: function block_block doesn't follow API , breaks usage of module_invoke('block','block','view'... was marked as a duplicate of this one.
Comment #15
Dave ReidIf this bug is still present in 7.x, we need to fix it there first with tests, then we can backport to 6.x. Moving to normal priority bug report since this doesn't completely screw up Drupal, but it is annonying.
Comment #16
shaneforsythe CreditAttribution: shaneforsythe commentedThe original patch will not work either as
function block_box_get($delta)
now only returns $block['content'] as well.
I have a questions about Gabor's comments in #7 , what exactly is the complaint holding back progress of this issue, which is almost 2 years old, and fails to follow core API documenttion.
1) Is it that you feel check_plain is being called twice?
In what regard do you see check_plain being called twice? To me and to the poster in #10 it does not appear plain_check is being called twice.
2) Or is the issue that plain_check is being called at all ala this comment, "hook_block() is not documented to expect a check_plain()-ed return value on the subject field,"
So ... the api doesn't expliticly state that check_plain should be used ...but it DOES state that $block['subject'] should be returned. Which is a grosser violation of the API , not returning the variable at all, or performing a sanity check on it, which may or may not be needed?
As to why check_plain is used at all ... its being used/called in block_list when the $block['subject'] is generated there ... so just following the precedence set there. If it is your opinion that it must be removed here, then surely it should be removed in block_list.
Comment #17
Gábor HojtsyHm, check_plain() is only called on the title, not the subject in block_list(). The title is user provided, the subject is code provided. It would be great to validate this with an independent review based on how the patch works, not just looking at the code, as I did.
Comment #18
shaneforsythe CreditAttribution: shaneforsythe commentedIn none of the tables (blocks or boxes) is there an actual field called 'subject'.
the $blocks variable does not have a 'subject' element until it is assigned the value from
ie: $block->subject does not exists before this ... check_plain is not called on $block['subject'] rather $block['subject'] is created using check_plain($block->title)
(The 'subject' that is returned via hook_block is actually the value 'title' in the database table)
This is exactly in keeping with the line in my diff
*edited
Now I see that block_list does a check, and if the user did not provide a title when the block was created, a default value of "none" was generated , and subject is set to blank (''). If that is desirable, then that same line in my diff can be changed to match exactly with
Comment #19
Jody LynnSee comment #15
Comment #20
cburschkaThere is no patch for D7 here.
Comment #21
alanburke CreditAttribution: alanburke commentedStill there in D7, though the function names have changed.
http://api.drupal.org/api/function/hook_block_view/7
States it should return the subject
http://api.drupal.org/api/function/block_block_view/7
Shows that it still does not.
Comment #22
sethcohn CreditAttribution: sethcohn commentedbumping to critical, to avoid having this bug continue into D8 or beyond.
This bug has remained unfixed since at least D5, despite numerous attempts to get it fixed.
What is so hard about making sure that the code actually does what the API says it should do? See #21.
Comment #23
alexanderpas CreditAttribution: alexanderpas commentedComment #24
sun.
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedAs far as I understand, this is nothing more then a documentation issue (that or that whole title vs subject feature of block_list / _block_render_blocks needs to be removed as well, and none of the patches posted in this issue tried to do that). Lowering priority accordingly.
Comment #26
sethcohn CreditAttribution: sethcohn commentedDamien, it's not just documentation. Core blocks do not follow the API that other blocks must (according to the spec), and changing the documentation to say "blocks may optionally return a subject" is a change in the API, which at this point is frozen, right? If someone had proposed solving this by changing the API documentation prior to the freeze, making it optional, yes, this could be then relabeled as a normal bug.
But the API _says_ it's a required field, and other modules treat it as that, since that's what the API says it is. The only exception is core, and isn't the point of a critical bug at this point in D7 to address things that don't do what the API says they should be doing?
Module authors hit this bug, and have to code around it. "Hmm, my other blocks all return ['subject'], why doesn't core? I'll have to code an exception here... But, the API says it's required. Wonder if there is a bug report on this... Oh, geez, since 2007, they've been trying to fix this? And now we'll have to wait till 2011-12?"
I understand you'd like to knock this off the list of D7 critical bugs, but I labeled it as critical, as I said, because it's been there since D5, and frankly, it's pretty absurd to not just patch this already, given that if it isn't fixed before D7 release, it'll stick around waiting for D8, at which point either it'll get patched OR someone can change the D8 API to say "optional". But neither fixes the problem for D7.
Comment #27
Damien Tournoud CreditAttribution: Damien Tournoud commentedD5 and D6 are not only API frozen but implementation frozen. As I said above, for D5 and D6, it's just a documentation bug. For D7, we can probably discuss. But as a minimum, we need to remove the "title vs subject feature of block_list() / _block_render_blocks()".
Frankly, there is nothing critical in that ("When a bug renders a module, a core system, or a popular function unusable.", from the handbook), it's nothing more then a minor annoyance (modules that want to call hook_block() directly need to copy/paste the three-lines of code from block_list()... big deal). Lowering again.
@sethcohn: why not contributing a patch? It would have taken about the same time as writing a four paragraphs post ;)
Comment #28
Damien Tournoud CreditAttribution: Damien Tournoud commentedComment #29
JohnAlbinHere's the updated patch for D7.
Comment #30
JacineI think I ran into this bug today creating a subtheme. I created some custom blocks while the Zen was the default, then switched to the Starter kit subtheme. As soon as I did that, titles stopped printing for the blocks. Neither the title nor the subject variables were populated for templates. The only way I could get them back was to re-save the blocks manually. I applied JohnAlbin's patch in #29 and it fixed the problem. It would be nice to get this in.
Comment #31
JacineHmm, actually it does not solve the problem. It causes the administrative title to print. Maybe it's not related? /me is confused.
Comment #32
alanburke CreditAttribution: alanburke commented#29: block-block-view-empty-subject-126070-29.patch queued for re-testing.
Comment #33
sunPatch seems to make sense, but we need a test that proves it.
Comment #34
moshe weitzman CreditAttribution: moshe weitzman commentedWow, this code is old school. We should be returning a render array as block content. This code returns a string.
Comment #35
manarth CreditAttribution: manarth commentedThis patch:
- provides the correct block title (not the 'info' field, which is the title displayed to administrators in admin/structure/blocks)
- provides block content as a renderable array
- adds test coverage to check block_block_view
Comment #36
sunI wonder whether this is the correct function signature... $delta should have no default, I think?
Shouldn't subject also be a renderable array or am I missing something? I case I do, let's clarify in an inline comment, please.
Powered by Dreditor.
Comment #37
manarth CreditAttribution: manarth commentedYou're right - the documentation gives: hook_block_view($delta = '')
I've used the same signature as provided by _block_render_blocks.
Given that title can only be a single element, I think a string is fine - but if it's better to make subject a renderable array, this change should probably be applied to _block_render_blocks too.
Patch attached fixes the function signature and adds an inline comment.
Comment #38
JohnAlbinThe tweak to the function definition is inline with the docs for hook_block_info(). And the test indeed checks that block_block_view() is returning the proper title.
I tested the patch on my system and it works fine.
Comment #39
sunThis should actually be a db_select() query, which uses ->addTag('translatable')
Comments should form a proper sentence and thus start with a capital letter.
NULL ==
<none>
=== TRUEWe should use a type-agnostic comparison here.
Powered by Dreditor.
Comment #40
manarth CreditAttribution: manarth commentedNew patch address some of Sun's comment's in #39:
- query now uses db_select instead of db_query (with appropriate translatable tag)
- comment grammar addressed
I'm not sure I understand this comment: the subject field is testing for the presence of the string '<none>' rather than an empty string. If this remains an issue in this patch, can you post to clarify this?
Comment #41
JohnAlbinThe patch in #40 introduces a very minor whitespace issue on line 253. That needs to be fixed.
I'm not sure I understand the
NULL == <none> === TRUE
comment either. Do you mean we should check if $block->title is NULL in the database by doing this?Comment #42
manarth CreditAttribution: manarth commentedPatch re-rolled to remove whitespace (and an errant full-stop).
Re: the question of NULL...
The patch provides
This means that if the title is null, this will resolve to:
And check_plain(NULL) will return an empty string, so there's no functional difference, but I would argue that this code is more readable.
The code for the subject element matches code used elsewhere in the block module, and the approach should at least be consistent across the module.
Fundamentally, the code in core is currently broken against the documented hook_block_view which says that the 'subject' element is required:
Comment #43
JohnAlbinBah. Somehow the review I wrote yesterday got lost when I posted it on a crappy wifi network.
I agree with manarth's minor changes. RTBC again.
Comment #44
webchickWhile technically an API change, returning subject seems like a suitable "oversight in the API" thing to justify a change this late. The function already returns an array, and we're just adding another index to it, so that's low-impact.
However, I'm really not sure about switching from returning a string in $data['content'] to a renderable array. While I agree that's what we should ultimately do here, it's a change that will break themes and probably Views and anything else that's calling hook_block('view').
Let's get a re-roll here with just the actual bug fixed.
Comment #45
sunThat change to a renderable array is not an API change, because every facility that deals with the output of hook_block_view() has to properly account for strings and renderable arrays.
The effective result in a page is the same, since _block_render_blocks() manually checks for blocks returning a string and "converts" them into a renderable array:
An interesting difference, however, is that a few lines before that, _block_render_blocks() invokes
So this means that hook implementations of hook_block_view_alter() get a renderable array for custom blocks of Block module to work with, which is much better, easier, and consistent.
Cleaned up code and comments.
Comment #46
Dries CreditAttribution: Dries commentedI'll leave this for webchick to decide.
Comment #47
tom_o_t CreditAttribution: tom_o_t commented#45: drupal.block-block-view.45.patch queued for re-testing.
Comment #48
webchickI'm still a bit dubious that this won't break anything, despite reassurances to the contrary. But the hook not returning all bits of the info is a legitimate bug fix, so this is still on the table IMO. So I'll go ahead and put this in now so we have a week to catch it "just in case."
Committed #45 to HEAD. Thanks!
Marking down to 6.x for consideration, since Gábor seemed pretty interested, but that was awhile ago; not sure we can fix it there.
Comment #49
David_Rothstein CreditAttribution: David_Rothstein commentedI think this patch is incorrect and needs to be rolled back.
Apparently the goal here was to make Drupal slower? :)
Seriously, though, this is an easy mistake to make and I think we need a followup issue to document the D7 caching behavior of check_markup() better (and possibly change that behavior back in D8). But for now, we definitely need to roll back the above change.
In contrast, the block 'subject' is a default title provided by the block itself. If a block doesn't provide its own title, it's supposed to set the 'subject' to NULL in hook_block_view() (for example, as in http://api.drupal.org/api/drupal/modules--system--system.module/function...). It is not supposed to dig into the block system's database tables and try to figure out the user-provided title on its own; that is not its job.
The committed patch therefore results in an odd inconsistency. If I call:
I get back the title which the user entered for that block via the administrative UI. However, if I call:
Then even if user entered a title for that block via the administrative UI, I don't get that title back.
That inconsistency is a problem.
The only correct fix for the actual bug in this issue is to follow the example of http://api.drupal.org/api/drupal/modules--system--system.module/function... and set the block 'subject' to NULL (thereby preventing the possibility of PHP notices, since the existence of that array key is indeed documented), and leave it at that.
Comment #50
David_Rothstein CreditAttribution: David_Rothstein commentedHere's a rollback patch that switches to using $data['subject'] = NULL, as described above. I also made some documentation improvements surrounding this point, because it's definitely a bit of a tricky issue :)
This is now a performance fix as well, due to the check_markup() problem I noted above.
Comment #51
webchickThanks, David. Committed #50 to HEAD. Bumping this to D8 to discuss.
Though adding the "translatable" tag to this query might be a legitimate bug fix we need to handle in a separate issue.
Comment #52
JohnAlbinRight, which is why I would oppose this change for Drupal 6. For D7, as stated above, the block module was not following its own guidelines that block content needs to be in the form of a renderable array. Modules being ported to D7 should be expecting a renderable array here because we told them it should be one. It's a bug that it isn't one. Which is why we fixed it.
This is a very good point. We should fix it, not revert the whole patch though.
Right, overridable titles are for all kinds of blocks. The problem is that the Block module’s interface and code is a *********, so it re-uses the same damn database field for both user-overridden titles and for the default titles of custom blocks. (I plan on fixing this for D8.) But, make no mistake, those are two completely different features:
We're not overriding a NULL title when we create a custom block, we are providing the default title when creating the custom block. So the example you provide is not inconsistent at all. If I call:
I get back the default title for the custom block the user created. And if I call:
Then I get back the default title of the block that the module created.
In neither case am I requesting the user-overridden title. Your code comment that “block_block_view() provides an empty block subject, since custom blocks do not have default titles” is incorrect; custom blocks do have default titles.
That's why this patch was spot on. Well, except for the check_plain() bit. :-)
The current state of the code is just as wrong as it was before the issue was started. NULL is not the default title of the custom block.
Here's a rollback of the rollback. I left in David's useful docblock improvements and the check_plain() fix. I RTBC those changes of his.
@webchick You committed a patch that was never discussed, was not RTBC, and was not tested by the testbot. o_O Do you perhaps need a Cherry Coke infusion?
Comment #53
webchickI was on the fence about committing this in the first place. David raised legitimate concerns. Rolling back seemed like a sensible thing to do.
Not waiting for testing bot though, yes, must've been off my meds at that point. :)
Not putting this back until you guys reach consensus, and I'd much prefer to kick the entire thing to D8 because it's a contentious change, a late change, and we need to get D7 done.
Comment #54
webchickComment #55
David_Rothstein CreditAttribution: David_Rothstein commentedYeah, I was a little scared to see that committed without the testbot too. I certainly hadn't run the tests locally :)
@JohnAlbin, I guess I still don't understand why you think the two types of blocks are different. Whether it's a custom block or not, there is always a field on the block configuration form called "Block title", and whatever you type in that field gets stored in the exact same place in the database. So if I have two different blocks, let's say one is the search block provided by search.module and the other is a custom block provided by block.module, and if I type in "My block title" in the above form field for both of them, I don't see why invoking hook_block_view() on one of them should return "My block title" but the other one shouldn't...
I agree the whole situation is a bit of a mess and there is room for big changes in D8, but with the way it works now, I don't see how it makes sense to treat block.module's implementation of hook_block_view() differently just because it lives in the same module as the hook it is implementing.
Well, I'm not totally opposed to that change, but I would disagree that it's a bug. The guidelines say it's preferable to be a renderable array, not that it must be one. And there are tons of other hook_block_view() implementations in core that (unfortunately) still return a string. In this case, the benefit of moving to an array isn't too big; it's not like we got a nice rich renderable array from it, rather we just went from $data['content'] containing a string to $data['content']['#markup'] containing the exact same string. So although I'm not directly opposed to it, I didn't see the point this late in D7, when it at least has the potential to break something.
Comment #56
SilviaT CreditAttribution: SilviaT commentedsub
Comment #57
Nikdilis CreditAttribution: Nikdilis commentedAny News concerning this issue that is a major pain in the ass? (this is why I think that the priority should be set to major)
See also: http://drupal.org/node/957038 - Regression: There is no (resonable) way to programmatically render a block.
Comment #58
David_Rothstein CreditAttribution: David_Rothstein commentedSince there are plenty of workarounds, this is not major.
And a couple of us have argued above that this is not even a bug... Or at least, not a bug the way it's been phrased and implemented so far in this issue.
What might be a legitimate way to title this issue is "There is no easy way to get the user-provided title of a block". That is, this code:
which is currently buried in _block_render_blocks(), should be abstracted out somehow so there's an easy way to ask for the block title that will be rendered.
But this is not a problem limited to custom blocks; the same issue applies to all blocks, so it should be fixed consistently for all blocks.
This is indeed very closely related to #957038: Regression: There is no (resonable) way to programmatically render a block or may require that as a prerequisite.
Comment #59
kscheirerI agree with David.
Comment #73
larowlanThis was resolved when we moved blocks to config-entities.
Here's the implementation of $block->label();
Comment #74
larowlan@quietone pointed to #1535868: Convert all blocks into plugins for where we moved to plugins for blocks