We have a ton of issues related to block titles, and we've been working ourselves up into an ineffectual tizzy over them for several months now. I'm hoping this issue can fix a lot of that by clarifying the undergirding issues, proposing some basic API & responsibility changes, and make princess happy in the process.

In all the issues I've read, there's no one place that we've clearly outlined the requirements for 'titles' on blocks. So, let me start with that:

  • We need a listing label for the UI at admin/structure/block/list/block_plugin_ui:bartik/add (in Bartik) - that is, the list of block plugins + derivatives. At this point, all we have access to is data from the annotation, and we should endeavor to be content with it.
  • We need an administrative label for the UI at admin/structure/block. At this point, we have a block plugin + configuration/block entity, but no context (that is, Request/route)-based data. We should also show this title in the UI regardless of the state of the 'Show title' checkbox. It's also being discussed in #1875252: [META] Make the block plugin UI shippable and implemented in #2014191: Implement the block-by-theme listing page, that two columns are presented: the frontend-facing title, and the administrative label. For the UI we have, I like this two-column suggestion.
  • We need the frontend-facing title for the block, for use at request time. Here we have everything - the block plugin, the configuration/block entity, and the real Request context in which we're operating. We also need to respect the 'Show title' checkbox in this case.
  • We also need to be able to accommodate showing the fully contextualized frontend title of the block for non-request time purposes like previews, regardless of whether the state of the 'Show title' checkbox.
  • Currently, the machine name is also conflated with the rest of this system, in that it is initialized to be the admin label declared on the annotation. This is silly, and I have a fix for it in #2022897: Shift responsibility for creating unique config id/machine name off of (views) block plugins and onto BlockFormController that disentangles machine name from title by adding a new BlockPluginInterface method. So let's not talk about machine name AT ALL in here, please :)

That is a maximum of four "titles" for a block: listing, administrative, context-free frontend, frontend.

Right now, we collapse listing and administrative together, using the 'admin_label' from the annotation. This does have the drawback of hiding the frontend title, albeit a context-free version of it, from the user. If we go with the dual-column approach, then we get both the context-free frontend and administrative, and it should be hunky-dory. Now, we sometimes think we see the frontend title in the administrative context because we default the 'label' from the 'admin_label' right now...but that horrible little masquerade just makes the whole thing way more confusing by hiding that there's a difference between the administrative and context-free frontend titles.

The rest of the issues stem from the fact that we - almost proudly - make no distinction between context-free frontend and real frontend title. We do that in large part because, somewhere along the way, we got the idea that creating a block title was a configuration task, and not something the block plugin knows anything about. Views is butting hard into this now, with the latest (and most productive issue, I think) being #1957276: Let users set the block instance title for Views blocks in the Block UI. I think the only problem with that issue is that its focus is too narrow, because...

When it comes to block titling, Views is not the exception. It is the rule.

In all these months of back-and-forth we've had about "what IS the block, the entity or the plugin?", we've lost sight of a crucial reality: Blocks are not user-created content. (Not even custom blocks are - the UNDERLYING entity is user-created content, but not the block plugin derivative spawned from it. Blocks are portable bits of rendering logic that can be configured to behave in slightly different ways. They often pass through to grab user-created content, but they themselves are not the content. They are rendering logic. In fact, the little masquerade of defaulting 'admin_label' into 'label' is evidence of this: even though we're not admitting it, we clearly already believe that the block has a good idea of what its title should be. As such, it is perfectly reasonable to expect block (plugins) to have opinions about what ALL of their output is. Including how they are titled, in all its variations. And for a domain object to "have an opinion" about something, that means it has a method. So...

This patch introduces four new methods to BlockPluginInterface:

  • getDefaultTitle() - the block returns a raw, untokenized title that is available without any configuration or contextual data injected (not that core is worrying about that yet). In other words, this is where the block plugin reports what it thinks the user-facing title should be.
  • getRawTitle() - the block returns a raw, untokenized title. The plugin should be able to produce this title without any contextual data injected, but can safely assume it is acting on a properly instanced block (that is, configuration have been injected). This will just call back to getDefaultTitle() in most cases; the use case we have for this in core is so that the block can let a title override entered by the user supercede its own default.
  • getTitle() - returns the fully-processed title string, exactly as it should be passed along for final output to the user (though still in need of escaping). This requires both an instanced block and contextual data to be present.
  • isTitleVisible() - returns a boolean indicating whether or not the title should be visible in a final frontend rendering scenario.

Note that, conceptually, the 'admin_label' annotation property is kinda of part of this API, as it should be considered the sole source for the administrative listing label use cases, as described above.

The code should make it clear that we still allow users to "set titles" on blocks - we're just changing the paradigm from it BEING the title, to it being a title override. If the user does not explicitly choose to make use of that override, then the native title logic from the block itself is used. And, unlike now, it's easy to allow the user to stop overriding the block and return to the native default supplied by the block plugin. merlinofchaos alluded to this in #1957276-38: Let users set the block instance title for Views blocks in the Block UI, though I think he should have been more forceful about it: the 'override title' mechanism is one we've used in Panels for years, and it's one of the things that actually works WELL. Combine it with an interaction flow akin to the one proposed by yoroy (the third one there), and we have a winner.

Once we do this, we can have calling code stop making the shitty assumption that everything extends from BlockBase, or at least utilizes the exact same datastructure rules. That means that template_preprocess_block() can call methods instead of inspecting magical keys in a stupid freakin array. Much more importantly, though, ViewsBlocks no longer need to set their title on $configuration['label'] (because THAT makes sense) during build() - nor will any other more complex blocks that are written in contrib (or princess). This is rather important because the implicit reliance on the 'label' array key effectively makes the BlockPluginInterface a lie.

Note that I'm not proposing we necessarily fix all of the UIs to be all happy-perfect in this issue. We should change the API in this issue, update things where necessary to get tests green, then fix the UIs in followups/the existing open issues once this is in.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sdboyer’s picture

no tests on this at all, and no even basic refactoring of the wrapping systems. there shouldn't NEED to be too many additional changes like that for just this API patch - most of it can be pushed to followups - but this gets the new methods onto the BlockPluginInterface and implements them on BlockBase, and should clearly demonstrate how i intend all these to hang together.

sdboyer’s picture

Issue tags: +VDC

adding VDC tag

sdboyer’s picture

Title: Add title-related methods to BlockPluginInterface to resolve many issues » Add title-related methods to BlockPluginInterface to help clarify and resolve many issues
Status: Active » Needs review

and that, too.

Status: Needs review » Needs work
Issue tags: -Needs tests, -VDC, -happy princess API

The last submitted patch, block-titles-2025649-1.patch, failed testing.

sdboyer’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +VDC, +happy princess API

#1: block-titles-2025649-1.patch queued for re-testing.

sdboyer’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

So just to understand I would like to write down how the views implementation would look like:

  • getDefaultTitle(): 'View Block', just because we don't have more context available.
  • getRawTitle(): 'The title of the view, which is configured via the UI'
  • getTitle(): The actual title of the view after it was rendered completely (which maps to $view->getTitle()),
    so it might contain contextual filter values.
sdboyer’s picture

not quite. all the methods in the proposed interface are dedicated to creating titles that are ultimately intended for use on the frontend. i should have included the 'admin_label' annotation value as part of the "API", as it does serve a valid role - but that's the only thing that contains an administratively-oriented title. THAT'S the thing that (if anything) should be 'Views Block'.

i've updated the issue summary accordingly.

you could/should do more with getDefaultTitle(). because the specific view+display that's being targeted by a ViewsBlock is defined as a derivative (and therefore is natively a part of the block plugin itself), everything that is statically configured on that view+display should be available to getDefaultTitle(). so it should be possible to provide a title that is, in some way, specific to that view+display.

the difference between getRawTitle() and getDefaultTitle() is that we can rely on block configuration data (i.e., a Block entity) being present. so the difference between the two is predicated on whatever choice ViewsBlock ends up offering to the user in its configuration form. if the ViewsBlock configuration form does not allow the user to manually change the title, either directly (via a title override) or indirectly (via some other magical setting toggle that somehow mutates the block output), then getRawTitle() and getDefaultTitle() should produce the same output.

i'll use RFC terminology: "If a block plugin does not allow the user to modify its title in any way, then all instances of that block SHOULD return the same value for getDefaultTitle() and getRawTitle().

i see no reason that Views would be an exception to this rule.

and yes, getTitle() can expect to produce a title result after build() has happened. while it should be avoided if possible, getTitle() implementations may need to trigger $this->build(), or some shared preparation logic called by that method, in order to fully produce the correct title. it is also at the discretion of the block plugin to decide whether or not to use the titles produced by the default/raw methods in deciding what its final title output would be. again, RFC language:

"The title produced by BlockPluginInterface::getTitle() implementations SHOULD be based on or derived from the output of BlockPluginInterface::getRawTitle()."

Views IS an exception to this rule, at least in some cases (contextual filter title overrides, and the empty title) since the only way to correctly get a title for a view is calling $view->getTitle(). If ViewsBlock does give the ability to set titles to users on the block config, though, then i'd say that that should be considered a replacement for that views+display's internal title, which is, under normal circumstances subject, interpolated with view results and returned by $view->getTitle().

sdboyer’s picture

Issue summary: View changes

Updated issue summary.

dawehner’s picture

One additional comment. I want to be sure that the block object is still the same, when getTitle() is called, so you can leveverage some information.

+1 in general.

dawehner’s picture

#1: block-titles-2025649-1.patch queued for re-testing.

sdboyer’s picture

ok, here's a patch that converts a couple of things:

  • the BlockRenderController now calls getTitle() instead of pulling the data straight from the configuration.
  • BlockBase::form() calls $this->getDefaultTitle() when no title is set, rather than getting the admin_label annotation property directly. getDefaultTitle() currently just gets it from the admin_label property, but it's still a better separation. now, i believe that whole label interaction needs to change (it needs to be an override), but changing the UI should be out of scope for this issue.
  • ViewsBlock no longer forces its title onto the configuration for magical pickup later: it now implements getTitle(), which it grabs from $view->getTitle(). it has to call build() in order to ensure that's present, but that's a known, unavoidable issue and is no regression from what we have now.

let's see how many tests blow up!

i'll add some unit tests for these new methods in the next round, once testbot tells gives me a hint where other bodies might be. i haven't checked any block implementations other than Views so far.

sdboyer’s picture

ehh, quite a delayed crosspost there, sorry.

One additional comment. I want to be sure that the block object is still the same, when getTitle() is called, so you can leveverage some information.

yep, it definitely is. i know of no place where we're recreating multiple instances of the block plugin, and if we are, then we're probably doing it quite wrong. my patch in #10 definitely relies on it being the same instance.

dawehner’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockRenderController.phpundefined
@@ -47,7 +47,7 @@ public function viewMultiple(array $entities = array(), $view_mode = 'full', $la
+        $build[$entity_id]['#configuration']['label'] = check_plain($plugin->getTitle());

Let's use String::format() for now.

+++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.phpundefined
@@ -141,4 +148,15 @@ public function generateBlockInstanceID(EntityStorageControllerInterface $manage
+    if (is_null($this->outputTitle)) {
+      $this->build();
+    }

Maybe a comment would be nice, to explain why it has to be called.

sdboyer’s picture

err...i switched to String::checkPlain(). not sure why i'd use String::format(), when i'd just be doing so to have it pass through to the check plain method.

in any case, done both of those.

Status: Needs review » Needs work
Issue tags: -Needs tests, -VDC, -happy princess API

The last submitted patch, block-titles-2025649-13.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +Needs tests, +VDC, +happy princess API

#13: block-titles-2025649-13.patch queued for re-testing.

sdboyer’s picture

oh man, green! sweet. OK, time to think about where else this API change should be applied...

dawehner’s picture

Yeah I was really tired. Do we still need tests?

sdboyer’s picture

@dawehner - yeah, we need some unit tests for the new methods, still. i'd say that Views also probably needs some tests to clearly verify its own expected behavior with respect to this API.

dawehner’s picture

dawehner’s picture

On #2032535: Resolve 'title' using the route and render array we are adding a new interface which has the request available all the time. Maybe we should do that here as well?

EclipseGc’s picture

#13: block-titles-2025649-13.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Needs tests, +VDC, +happy princess API

The last submitted patch, block-titles-2025649-13.patch, failed testing.

sdboyer’s picture

Status: Needs work » Needs review
FileSize
7.14 KB

ok, this was just a straight reroll...i've only looked cursorily at the changes in the last month, but nothing immediately jumped out at me as a reason why this wouldn't work. let's see what testbot says.

tim.plunkett’s picture

Straight reroll. Needs tests!

tim.plunkett’s picture

  1. +++ b/core/modules/block/lib/Drupal/block/BlockBase.php
    @@ -93,7 +93,7 @@ public function buildConfigurationForm(array $form, array &$form_state) {
    -      '#default_value' => !empty($this->configuration['label']) ? $this->configuration['label'] : $definition['admin_label'],
    +      '#default_value' => !empty($this->configuration['label']) ? $this->configuration['label'] : $this->getDefaultTitle(),
    
    @@ -182,4 +182,42 @@ public function getMachineNameSuggestion() {
    +  public function getRawTitle() {
    ...
    +    return isset($this->configuration['label']) ? $this->configuration['label'] : $this->getDefaultTitle();
    

    Shouldn't this use the method?

  2. +++ b/core/modules/views/lib/Drupal/views/Plugin/Block/ViewsBlock.php
    @@ -122,4 +122,19 @@ public function getMachineNameSuggestion() {
    +  public function getTitle() {
    +    // The only canonical way to build the title for a view is to ask the view
    +    // for it directly, once the view has been executed. To avoid executing the
    +    // view unnecessarily, we do it only in build(), then set a property for use
    +    // here. So, if that property is not set, we must call build() to set it.
    +    if (is_null($this->outputTitle)) {
    +      $this->build();
    +    }
    +
    +    return $this->outputTitle;
    

    This is fine for me

benjy’s picture

Used the getConfiguration() method as suggested by tim.plunkett and fixed a style error with a comment.

benjy’s picture

Issue summary: View changes

brought 'admin_label' annotation prop a little more to the forefront.

dawehner’s picture

27: block-titles-2025649-27.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 27: block-titles-2025649-27.patch, failed testing.

ekes’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
7.25 KB

Re-roll #27 changing core/modules/block/lib/Drupal/block/BlockRenderController.php for core/modules/block/lib/Drupal/block/BlockViewBuilder.php for #2097903: Rename EntityRenderController to EntityViewBuilder.

moshe weitzman’s picture

Latest patch is green. Anyone have further comments? I'm gonna start working on cache tags for blocks so sorting some of these issues now would be good.

benjy’s picture

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 32: block-titles-2025649-32.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
7.48 KB

Arghh

benjy’s picture

Status: Needs review » Needs work
+++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
@@ -9,7 +9,9 @@
+ ¶

White space needs removing when this get's rolled again and we still need tests.

dawehner’s picture

this still would be nice!

martin107’s picture

Status: Needs work » Needs review
FileSize
7.19 KB

Reroll.

tim.plunkett’s picture

  1. +++ b/core/modules/block/lib/Drupal/block/BlockBase.php
    @@ -126,7 +126,7 @@ public function buildConfigurationForm(array $form, array &$form_state) {
    -      '#default_value' => !empty($this->configuration['label']) ? $this->configuration['label'] : $definition['admin_label'],
    +      '#default_value' => !empty($this->configuration['label']) ? $this->configuration['label'] : $this->getDefaultTitle(),
    

    This should use getRawTitle

  2. +++ b/core/modules/block/lib/Drupal/block/BlockBase.php
    @@ -293,6 +293,46 @@ public function getCacheTags() {
    +  public function getDefaultTitle() {
    ...
    +    return $definition['admin_label'];
    ...
    +  public function getRawTitle() {
    ...
    +    return isset($configuration['label']) ? $configuration['label'] : $this->getDefaultTitle();
    ...
    +  public function getTitle() {
    ...
    +  public function isTitleVisible() {
    ...
    +    if (!isset($this->configuration['label_display'])) {
    

    Can we rename all of these to get*Label()? Everything they use is about labels...

  3. +++ b/core/modules/block/lib/Drupal/block/BlockViewBuilder.php
    @@ -13,6 +13,7 @@
    +use Drupal\Core\Entity\Plugin\DataType\StringItem;
    

    Pretty sure this isn't needed.

Status: Needs review » Needs work

The last submitted patch, 37: block-titles-2025649-37.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
7.77 KB
7.62 KB

@tim.plunkett Thanks for the review...

addressed all issue from #38
Many tests look better..

( Removed lots of extra unnecessary 'use' statements )

Fixed :-
DisplayBlockTest
CommentBlockTest
DisplayTest
BasicTest

Status: Needs review » Needs work

The last submitted patch, 40: block-titles-2025649-40.patch, failed testing.

tim.plunkett’s picture

+++ b/core/modules/block/lib/Drupal/block/BlockBase.php
@@ -126,7 +125,7 @@ public function buildConfigurationForm(array $form, array &$form_state) {
-      '#default_value' => !empty($this->configuration['label']) ? $this->configuration['label'] : $definition['admin_label'],
+      '#default_value' => !empty($this->configuration['label']) ? $this->configuration['label'] : $this->getRawLabel(),

No, what I meant was that this should just be '#default_value' => $this->getRawLabel(), since it handles this check anyway.

martin107’s picture

Status: Needs work » Needs review
FileSize
8.47 KB
2.59 KB

#40 No longer applies ... so reroll.

The Interdiff is for changes made after...

This patch does not address the existing errors.

BlockBase::label() and BlockBase:getLabel() are functionally equivalent so label() is removed.

I have searched the code and run a few tests hunting for calls to label() but I can find only 1... looking to testbot to give the definitive answer...

Also changed a few instances of 'title' in comments to 'label'

Status: Needs review » Needs work

The last submitted patch, 43: block-titles-2025649-43.patch, failed testing.

dawehner’s picture

Will this one ever happen?

cilefen’s picture

This needs a change record because it expands an interface, and a beta evaluation.

tim.plunkett’s picture

Version: 8.0.x-dev » 9.x-dev
Category: Bug report » Task

Not a bug, and AFAICS not doable in D8

catch’s picture

Version: 9.x-dev » 8.3.x-dev
Priority: Major » Minor

Some of this could be done in 8.3.x with bc.

1. Add a new interface, have BlockBase implement it.

2. Check implements and use the method if available.

3. E_USER_DEPRECATED in the fallback.

While the bc policy allows additions to interfaces where there's a base class, I don't think there's enough benefit here to justify that.

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

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now 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.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now 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.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now 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.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now 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.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.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.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now 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.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.