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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zostay’s picture

This 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

RobRoy’s picture

Version: 5.x-dev » 6.x-dev

Moving to HEAD.

bdragon’s picture

FileSize
836 bytes

Free reroll courtesy patch bingo.

birdmanx35’s picture

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

sethcohn’s picture

Version: 6.x-dev » 5.x-dev
Priority: Normal » Critical

Happened 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:

$box = block_box_get($delta);
$data = array(
        'subject' => check_plain($box['title']),
        'content' => check_markup($box['body'], $box['format'], FALSE),
      );

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

If $op is 'view', return an array which must define a 'subject' element and a 'content' element defining the block indexed by $delta.

yet, the block module itself fails to meet this spec for the hook.

drumm’s picture

Version: 5.x-dev » 6.x-dev

The patch is for Drupal 6.x and should be reviewed/committed there first.

Gábor Hojtsy’s picture

There 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?

catch’s picture

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

Bumping to D7.

RobRoy’s picture

I'm not sure why I put that in there. It can probably be safely removed.

pwolanin’s picture

Looks 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():


          if ($block->title) {
            // Check plain here to allow module generated titles to keep any markup.
            $block->subject = $block->title == '<none>' ? '' : check_plain($block->title);
          }
          if (!isset($block->subject)) {
            $block->subject = '';
          }

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

Robin Monks’s picture

Status: Needs review » Reviewed & tested by the community

Patch seems to work, block simpletest passes "58 passes, 0 fails, 0 exceptions". RTBC.

Robin

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Doesn't look like Gabor's comments in #7 were addressed.

shaneforsythe’s picture

Version: 7.x-dev » 6.8
FileSize
688 bytes

Not 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

# If $op is 'view': return an array which must define a 'subject' element and a 'content' element defining the block indexed by $delta.

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

$block = module_invoke('block', 'block', 'view', 1);
$output .= $block['subject'];
$output .= $block['content'];

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.

Gábor Hojtsy’s picture

Dave Reid’s picture

Version: 6.8 » 7.x-dev
Priority: Critical » Normal

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

shaneforsythe’s picture

Version: 7.x-dev » 6.8
Priority: Normal » Critical

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

Gábor Hojtsy’s picture

Status: Needs work » Needs review

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

        if (isset($block->content) && $block->content) {
          // Override default block title if a custom display title is present.
          if ($block->title) {
            // Check plain here to allow module generated titles to keep any markup.
            $block->subject = $block->title == '<none>' ? '' : check_plain($block->title);
          }
          if (!isset($block->subject)) {
            $block->subject = '';
          }
          $blocks[$block->region]["{$block->module}_{$block->delta}"] = $block;
        }
shaneforsythe’s picture

In 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

$block->subject = $block->title == '<none>' ? '' : check_plain($block->title);

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

+      $data['subject'] = check_plain($block->title);

*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

+    $block->subject = $block->title == '<none>' ? '' : check_plain($block->title);
Jody Lynn’s picture

Version: 6.8 » 7.x-dev
Priority: Critical » Normal

See comment #15

cburschka’s picture

Status: Needs review » Active

There is no patch for D7 here.

alanburke’s picture

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

sethcohn’s picture

Priority: Normal » Critical

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

alexanderpas’s picture

Issue tags: +D7 API clean-up
sun’s picture

.

Damien Tournoud’s picture

Priority: Critical » Normal

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

sethcohn’s picture

Priority: Normal » Critical

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

Damien Tournoud’s picture

D5 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 ;)

Damien Tournoud’s picture

Priority: Critical » Normal
JohnAlbin’s picture

Status: Active » Needs review
FileSize
1001 bytes

Here's the updated patch for D7.

Jacine’s picture

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

Jacine’s picture

Hmm, actually it does not solve the problem. It causes the administrative title to print. Maybe it's not related? /me is confused.

alanburke’s picture

sun’s picture

Issue tags: +Needs tests

Patch seems to make sense, but we need a test that proves it.

moshe weitzman’s picture

Wow, this code is old school. We should be returning a render array as block content. This code returns a string.

manarth’s picture

This 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

sun’s picture

+++ modules/block/block.module	22 Aug 2010 15:41:42 -0000
@@ -243,9 +243,12 @@
+function block_block_view($delta = 0) {

I wonder whether this is the correct function signature... $delta should have no default, I think?

+++ modules/block/block.module	22 Aug 2010 15:41:42 -0000
@@ -243,9 +243,12 @@
+    'subject' => $block->title == '<none>' ? '' : check_plain($block->title),
+    'content' => array('#markup' => check_markup($block->body, $block->format)),

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.

manarth’s picture

"I wonder whether this is the correct function signature"

You're right - the documentation gives: hook_block_view($delta = '')

"Shouldn't subject also be a renderable array or am I missing something?"

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.

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

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

sun’s picture

Status: Reviewed & tested by the community » Needs work
+++ modules/block/block.module	22 Aug 2010 17:17:13 -0000
@@ -243,9 +243,13 @@
+  $block = db_query('SELECT bc.body, bc.format, b.title FROM {block_custom} bc LEFT JOIN {block} b ON (bc.bid = b.delta) WHERE bc.bid = :bid', array(':bid' => $delta))->fetchObject();

This should actually be a db_select() query, which uses ->addTag('translatable')

+++ modules/block/block.module	22 Aug 2010 17:17:13 -0000
@@ -243,9 +243,13 @@
+    // subject uses the same signature as _block_render_blocks().

Comments should form a proper sentence and thus start with a capital letter.

+++ modules/block/block.module	22 Aug 2010 17:17:13 -0000
@@ -243,9 +243,13 @@
+    'subject' => $block->title == '<none>' ? '' : check_plain($block->title),

NULL == <none> === TRUE

We should use a type-agnostic comparison here.

Powered by Dreditor.

manarth’s picture

Status: Needs work » Needs review
FileSize
2.6 KB

New 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

+++ modules/block/block.module 22 Aug 2010 17:17:13 -0000
@@ -243,9 +243,13 @@
+    'subject' => $block->title == '<none>' ? '' : check_plain($block->title),

NULL == === TRUE

We should use a type-agnostic comparison here.

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?

JohnAlbin’s picture

Status: Needs review » Needs work

The 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?

'subject' => (empty($block->title) || $block->title == '<none>') ? '' : check_plain($block->title),
manarth’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
2.91 KB

Patch re-rolled to remove whitespace (and an errant full-stop).

Re: the question of NULL...

The patch provides

'subject' => $block->title == '<none>' ? '' : check_plain($block->title),

This means that if the title is null, this will resolve to:

'subject' => check_plain(NULL),

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:

Return value

An array containing required elements 'subject' (the block's localized title) and 'content' (the block's body). The 'content' element may be a renderable array (preferable) or rendered HTML content.

JohnAlbin’s picture

Status: Needs review » Reviewed & tested by the community

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work

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

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
3 KB

That 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:

        // Normalize to the drupal_render() structure.
        if (is_string($block->content)) {
          $block->content = array('#markup' => $block->content);
        }

An interesting difference, however, is that a few lines before that, _block_render_blocks() invokes

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

        if (isset($cid)) {
          cache_set($cid, $array, 'cache_block', CACHE_TEMPORARY);
        }

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.

Dries’s picture

I'll leave this for webchick to decide.

tom_o_t’s picture

#45: drupal.block-block-view.45.patch queued for re-testing.

webchick’s picture

Version: 7.x-dev » 6.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

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

David_Rothstein’s picture

Version: 6.x-dev » 7.x-dev
Status: Patch (to be ported) » Needs work

I think this patch is incorrect and needs to be rolled back.

  1. For starters, @webchick is right that the string-to-array change could break something (anyone implementing hook_block_view_alter and targeting these blocks specifically would have their code broken). Maybe it's unlikely, but that change wasn't even related to this issue anyway.
  2. -  $data['content'] = check_markup($block->body, $block->format, '', TRUE);
    +      '#markup' => check_markup($block->body, $block->format),
    

    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.

  3. Most important, though, I think the premise of this issue is flawed. User-overridable titles (stored in $block->title) are a feature that the block system provides for all blocks, not just custom ones.

    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:

    module_invoke('block', 'block_view', $delta);
    

    I get back the title which the user entered for that block via the administrative UI. However, if I call:

    module_invoke('some_other_module', 'block_view', $delta);
    

    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.

David_Rothstein’s picture

Status: Needs work » Needs review
Issue tags: +Performance
FileSize
4.37 KB

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

webchick’s picture

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

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

JohnAlbin’s picture

Version: 8.x-dev » 7.x-dev
Status: Needs review » Reviewed & tested by the community
FileSize
2.54 KB

For starters, @webchick is right that the string-to-array change could break something (anyone implementing hook_block_view_alter and targeting these blocks specifically would have their code broken).

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

Seriously, though, this [check_plain() error] is an easy mistake to make and I think we need a followup issue to document the D7 caching behavior of check_markup() better

This is a very good point. We should fix it, not revert the whole patch though.

User-overridable titles (stored in $block->title) are a feature that the block system provides for all blocks, not just custom ones.

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:

  1. User-overridable titles
  2. Custom blocks (with content and title)

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:

module_invoke('block', 'block_view', $delta);

I get back the default title for the custom block the user created. And if I call:

module_invoke('some_other_module', 'block_view', $delta);

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?

webchick’s picture

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

webchick’s picture

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

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

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

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.

SilviaT’s picture

sub

Nikdilis’s picture

Priority: Normal » Major

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

David_Rothstein’s picture

Version: 7.x-dev » 8.x-dev
Priority: Major » Normal

Since 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:

        // Override default block title if a custom display title is present.
        if ($block->title) {
          // Check plain here to allow module generated titles to keep any
          // markup.
          $block->subject = $block->title == '<none>' ? '' : check_plain($block->title);
        }
        if (!isset($block->subject)) {
          $block->subject = '';
        }

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.

kscheirer’s picture

Status: Needs review » Needs work

I agree with David.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 3974c6c on 8.3.x
    #126070 by RobRoy, JohnAlbin, manarth, et al: Fixed module_invoke('block...
  • webchick committed b44b2c6 on 8.3.x
    #126070 follow-up by David_Rothstein: Rollback of previous patch; needs...

  • webchick committed 3974c6c on 8.3.x
    #126070 by RobRoy, JohnAlbin, manarth, et al: Fixed module_invoke('block...
  • webchick committed b44b2c6 on 8.3.x
    #126070 follow-up by David_Rothstein: Rollback of previous patch; needs...

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • webchick committed 3974c6c on 8.4.x
    #126070 by RobRoy, JohnAlbin, manarth, et al: Fixed module_invoke('block...
  • webchick committed b44b2c6 on 8.4.x
    #126070 follow-up by David_Rothstein: Rollback of previous patch; needs...

  • webchick committed 3974c6c on 8.4.x
    #126070 by RobRoy, JohnAlbin, manarth, et al: Fixed module_invoke('block...
  • webchick committed b44b2c6 on 8.4.x
    #126070 follow-up by David_Rothstein: Rollback of previous patch; needs...

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

  • webchick committed 3974c6c on 9.1.x
    #126070 by RobRoy, JohnAlbin, manarth, et al: Fixed module_invoke('block...
  • webchick committed b44b2c6 on 9.1.x
    #126070 follow-up by David_Rothstein: Rollback of previous patch; needs...

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

larowlan’s picture

Issue summary: View changes
Status: Needs work » Closed (works as designed)
Issue tags: +Bug Smash Initiative

This was resolved when we moved blocks to config-entities.

Here's the implementation of $block->label();

 /**
   * {@inheritdoc}
   */
  public function label() {
    $settings = $this->get('settings');
    if ($settings['label']) {
      return $settings['label'];
    }
    else {
      $definition = $this->getPlugin()->getPluginDefinition();
      return $definition['admin_label'];
    }
  }
larowlan’s picture

@quietone pointed to #1535868: Convert all blocks into plugins for where we moved to plugins for blocks