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 togetDefaultTitle()
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, ViewsBlock
s 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.
Comment | File | Size | Author |
---|---|---|---|
#43 | interdiff-40-43.txt | 2.59 KB | martin107 |
#43 | block-titles-2025649-43.patch | 8.47 KB | martin107 |
#40 | block-titles-2025649-40.patch | 7.62 KB | martin107 |
#40 | interdiff-37-40.txt | 7.77 KB | martin107 |
#37 | block-titles-2025649-37.patch | 7.19 KB | martin107 |
Comments
Comment #1
sdboyer CreditAttribution: sdboyer commentedno 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 onBlockBase
, and should clearly demonstrate how i intend all these to hang together.Comment #2
sdboyer CreditAttribution: sdboyer commentedadding VDC tag
Comment #3
sdboyer CreditAttribution: sdboyer commentedand that, too.
Comment #5
sdboyer CreditAttribution: sdboyer commented#1: block-titles-2025649-1.patch queued for re-testing.
Comment #5.0
sdboyer CreditAttribution: sdboyer commentedUpdated issue summary.
Comment #6
dawehnerSo just to understand I would like to write down how the views implementation would look like:
so it might contain contextual filter values.
Comment #7
sdboyer CreditAttribution: sdboyer commentednot 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 aViewsBlock
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 togetDefaultTitle()
. so it should be possible to provide a title that is, in some way, specific to that view+display.the difference between
getRawTitle()
andgetDefaultTitle()
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 choiceViewsBlock
ends up offering to the user in its configuration form. if theViewsBlock
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), thengetRawTitle()
andgetDefaultTitle()
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()
andgetRawTitle()
.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 ofBlockPluginInterface::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()
. IfViewsBlock
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()
.Comment #7.0
sdboyer CreditAttribution: sdboyer commentedUpdated issue summary.
Comment #8
dawehnerOne 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.
Comment #9
dawehner#1: block-titles-2025649-1.patch queued for re-testing.
Comment #10
sdboyer CreditAttribution: sdboyer commentedok, here's a patch that converts a couple of things:
BlockRenderController
now callsgetTitle()
instead of pulling the data straight from the configuration.BlockBase::form()
calls$this->getDefaultTitle()
when no title is set, rather than getting theadmin_label
annotation property directly.getDefaultTitle()
currently just gets it from theadmin_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 implementsgetTitle()
, which it grabs from$view->getTitle()
. it has to callbuild()
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.
Comment #11
sdboyer CreditAttribution: sdboyer commentedehh, quite a delayed crosspost there, sorry.
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.
Comment #12
dawehnerLet's use String::format() for now.
Maybe a comment would be nice, to explain why it has to be called.
Comment #13
sdboyer CreditAttribution: sdboyer commentederr...i switched to
String::checkPlain()
. not sure why i'd useString::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.
Comment #15
tim.plunkett#13: block-titles-2025649-13.patch queued for re-testing.
Comment #16
sdboyer CreditAttribution: sdboyer commentedoh man, green! sweet. OK, time to think about where else this API change should be applied...
Comment #17
dawehnerYeah I was really tired. Do we still need tests?
Comment #18
sdboyer CreditAttribution: sdboyer commented@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.
Comment #19
dawehnerI hate it, that we would add the same test filse as we would have in the other two related issues, which are both RTBC.
#1957346: Add some settings on the block display to allow overrides on the block instance configuration
#2022897: Shift responsibility for creating unique config id/machine name off of (views) block plugins and onto BlockFormController
Comment #20
dawehnerOn #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?
Comment #21
EclipseGc CreditAttribution: EclipseGc commented#13: block-titles-2025649-13.patch queued for re-testing.
Comment #23
sdboyer CreditAttribution: sdboyer commentedok, 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.
Comment #25
tim.plunkettStraight reroll. Needs tests!
Comment #26
tim.plunkettShouldn't this use the method?
This is fine for me
Comment #27
benjy CreditAttribution: benjy commentedUsed the getConfiguration() method as suggested by tim.plunkett and fixed a style error with a comment.
Comment #27.0
benjy CreditAttribution: benjy commentedbrought 'admin_label' annotation prop a little more to the forefront.
Comment #28
dawehner27: block-titles-2025649-27.patch queued for re-testing.
Comment #30
ekes CreditAttribution: ekes commentedRe-roll #27 changing
core/modules/block/lib/Drupal/block/BlockRenderController.php
forcore/modules/block/lib/Drupal/block/BlockViewBuilder.php
for #2097903: Rename EntityRenderController to EntityViewBuilder.Comment #31
moshe weitzman CreditAttribution: moshe weitzman commentedLatest 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.
Comment #32
benjy CreditAttribution: benjy commentedRe-rolled.
Comment #34
benjy CreditAttribution: benjy commentedArghh
Comment #35
benjy CreditAttribution: benjy commentedWhite space needs removing when this get's rolled again and we still need tests.
Comment #36
dawehnerthis still would be nice!
Comment #37
martin107 CreditAttribution: martin107 commentedReroll.
Comment #38
tim.plunkettThis should use getRawTitle
Can we rename all of these to get*Label()? Everything they use is about labels...
Pretty sure this isn't needed.
Comment #40
martin107 CreditAttribution: martin107 commented@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
Comment #42
tim.plunkettNo, what I meant was that this should just be
'#default_value' => $this->getRawLabel(),
since it handles this check anyway.Comment #43
martin107 CreditAttribution: martin107 commented#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'
Comment #45
dawehnerWill this one ever happen?
Comment #46
cilefen CreditAttribution: cilefen commentedThis needs a change record because it expands an interface, and a beta evaluation.
Comment #47
tim.plunkettNot a bug, and AFAICS not doable in D8
Comment #48
catchSome 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.