Problem/Motivation
This is a direct blocker for #2352155: Remove HtmlFragment/HtmlPage. Over there, we're working to make page rendering understandable again.
In HEAD, we MUST render the main content FIRST, because we need that for the title. i.e. we can't just render $page
, we must first render $page['content']
, because $page['content']['#title']
may define the page title, which we need to print as part of $page
's template.
This breaks the stack-based bubbling, because rendering $page['content']
(the main content) first separately means its bubbleable metadata (attachments, cache tags and #post_render_cache
callbacks) won't be part of the stack of metadata being bubbled when rendering $page
. The reason HEAD gets around this are: hacks that use globals to juggle things around, which we've been working to get rid of, plus drupal_render()
not yet being as strict as it should be (a non-recursive (root) call to drupal_render()
that does not result in an empty stack at the end of the root drupal_render()
call does not complain currently, but should throw an exception, because it means bubbleable metadata is being left behind).
As of #9, this is no longer true. It'd still be better if we could enforce this, but we can't. But we can convert many (most?) uses in Drupal 8 HEAD to no longer use $main_content['#title']
. Also see #7.
It also creates an utterly awful DX, the first testament of which is Drupal core itself: its *.routing.yml
files are littered with _title
attributes which are never ever used, because they're overridden by $main_content['#title']
($page['content']['#title']
)!
So: we think we define a title, but then dynamically override it. Or also: we think we override the title in a hook_form_alter()
and then it's never actually overridden due to a change elsewhere, and is then falling back to the static _title
. To top it off, the path-based breadcrumb builder will just look at every single path component along the way… including those that don't represent a valid path! E.g. /nl/node/1/translations/add/en/nl
, for which _title = 'Add'
is defined, but it's dynamically overridden using #title
. The breadcrumb builder then gets the titles for /nl/node/1/translations/add/en
and /nl/node/1/translations/add
, which is 'Add'
according to the static title, but in fact neither of those paths are *valid*! They both trigger exceptions. So we get a breadcrumb like Home > NODE TITLE > Translations > Add > Add
, but neither of the two last links work, and this fact is subtly hidden, in part thanks to the combination of _title
and #title
.
An incredibly large percentage of titles is currently broken in this way in Drupal 8 HEAD.
I'd argue something as fundamental as page titles for a CMS should work in a very clear, consistent way, and bubbling is clearly not the best way to achieve that.
Proposed resolution
Remove the ability to set a dynamic page title using #title
, always use _title
or _title_callback
.
Remaining tasks
Get green.
User interface changes
None. Except more consistent page titles for entity routes.
API changes
No longer able to set/override the page title using #title
on main content returned in a _content
or _form
controller
Comment | File | Size | Author |
---|---|---|---|
#22 | title-woes-2359901.22.patch | 87.99 KB | larowlan |
#1 | rm_title_bubbling-2359901-1.patch | 119.4 KB | Wim Leers |
Comments
Comment #1
Wim LeersThis patch won't be green, but it does the majority of the work.
A nice consequence is that all
_entity_view
and_entity_form
routes for example automatically get standardized titles, rather than the many inconsistent, missing and quite often plain broken page titles.Comment #3
dawehnerFirst, its a total valid statement to convert as many of those examples to use
_title
and_title_callback
, but let's play the realist here.The actual reason (not mentioned sadly in the IS)
why we not only added support for
_title/_title_callback
but especially used#title
, is really simple:We want to be able to change the title of a page/block/... depending on the content of the page/block. A really simple example is the frontpage "/node".
If you have no content yet, the title of it says "Welcome to Drupal". In other cases there is no title, but you easily can come up with many other examples.
A search page for example also might want to display the amount of results inside the title ...
There are other reasons why we render the main content first, for example we do have this function called
views_get_page_view()
which themers use all over the place to add classes to the page templatebased upon it. It can't have no results when we come to page rendering, or we should drop support for it entirely.
Is there a reason why we can't render all stuff in all "regions" first and then render the page/html itself?
So you start with rendering each block, and then extract the bubbleable data out of it and put that into the main page template.
I opened a dedicated test issue to ensure that things don't break under the hood, again: #2359931: Ensure that empty title support does not break.
Comment #4
dawehnerIn case we want to adapt the TitleResolverInterface we might want to expand the test coverage as well?
Just curious, but DO people really have to call check plain for their own, because many places in the patch doesn't do it ...
Let's not introduce shit here, entity_load() should not be used since quite a time.
What would other operations do, provide their own
getTitle()
method?Is it just me who thinks that bypassing FAPI here is a hack?
I love php :)
Here is one of these instances I would not trust, if possible.
It is kinda crazy that we have to execute the hook_help code twice now. Are you sure this won't be problematic in terms actual performance?
... I think the claim is not that the PathBasedBreadcrumbBuilder always works, its just the default implementation which works for most examples. If you need something more advanced you can always use a custom one, if you like. Maybe having a custom one for the translation page is something reasonable to do.
oh? We cannot return a string anymore?
As described earlier, this doesn't work. On top of that, this functionality does not belong into the View entity, because the View entity is just about storage. All logic is part of the ViewExecutable.
Is it just me, or are you really convinced to throw an exception as part of the title handling? With the current implementation of the TitleResolver this will bubble up to a 403 exception. Is this really what you wanted? This is afaik a functionality change ...
Comment #5
andypostThis is a serious API change at this stage! Suppose needs approval.
Overall the change is great step towards sanity of page building and unification of page titles!
The only caveat I see that some title callbacks will require to duplicate a logic of controller to set proper title.
The same we have for blocks that "build" their title within content, and probably this behaviour is used to set page title from the main content block(controller). So we could clearly document in change notice how to deal with that kind of changes with all recommended approaches.
Currently the "virtuality" of main content block mixed with controller's content (and #title) so we need better to separate this concerns
Let's unify that to getTile() - nice pattern
Do this makes the method required?
The comment is wrong.
... if no entity type defined as bundle for this type we use type name label, otherwise the label of bundle entity type.
Also "entity_load" needs injection of EM
Great! please change @type to @entity_type_label - this should help translation to get more context
Not clear how to extend this for other opertions
the same, and better use "Edit comment %title" as it was
this is a different pattern
Please reformat that to:
is this fine?
maybe use module handler somehow?
nice clean-up!
is there other way? loading NULL is not good
is different to common pattern.
maybe getTitle?
Comment #6
Berdir(Wanted to post this before all the other comments, but failed because crosspost)
I can see the problems, but this is also a DX regression in some cases, as it will be a lot more complicated to set the title in some cases?
At the same time it can also help with the confusion between _title and #title. Which has been a discussion point for #2287733: Configure page title per display. Which is a good example, I'm not sure how to move forward there..
A page can have multiple displays, and one is chosen and runtime based on visibility conditions (like user being logged in or not, but also context for a specific site like current node). So we want to avoid having to do that calculation twice *and* if we are currently on a different page, we might not be able to detect the same display?
A common requirement in the past was to have different labels for local tasks/links than the page title, but I guess that is no longer a problem as those are now always stored separately (like having Edit in the local task and Edit @title as page title then)
Comment #7
Wim LeersAs long as nobody can adequately answer dawehner's big question, I don't see how we can do what this issue aims to do:
At least, I personally don't know how to adequately answer that excellent question.
… assuming we cannot answer dawehner's excellent question, I think we should relabel this issue to
, because I think we all want simplicity whenever possible. dawehner is also in favor of that:Comment #8
catchYeah that looks like a valid use-case and I don't have another idea.
At the very least, it would need an alternative mechanism for setting the title based on the content, even if that mechanism has a different implementation than #title.
Another thing here is that the page title in that case is only going to be changed when viewing the page - for menu links the code setting #title won't shouldn't run - and that's a good thing because we don't want to be checking view results just when rendering a link etc.
Comment #9
Wim LeersThanks for confirming, catch. I'll leave this open for another day or so in its current state. If nobody is able to articulate an adequate answer to dawehner's question, I'm retitling this as said in #7.
Already degrading to major, because I found a way around the problem described in the IS, which was the sole reason for me even beginning this work. Since I found a work-around, this is no longer a blocker, hence it's also no longer critical.
Comment #10
aspilicious CreditAttribution: aspilicious commentedI also need to be able to dynamicly set the title in contrib. As long as we make that possible, I'm happy.
Comment #11
larowlanI have an idea about 7, digging
Comment #12
larowlanSo here's my idea - this patch will fail because I've only made a couple of conversions - but it works.
The main premise is to change the reponsibility for creating the HtmlFragment object, creating it earlier and letting the controller resolver pass it to controllers if they have an argument called fragment
Requires some minor changes to CacheableInterface but other than that - it seems to work.
Sample conversions for help and forum module included.
eg help page has this routing
And I made this change
And it works
Thoughts?
Comment #13
larowlanI have a niggling feeling that what I've done in the patch solves the need for #title but may not solve the bigger problem, but not sure
Comment #14
larowlanSo my patch does solve this, $page['content'] is no different to $page['sidebar_first'] - sweet
Comment #15
larowlanso keeping going down that path
Comment #16
larowlanInterdiff against #12
Comment #18
larowlandoh
Comment #20
larowlanChanges to formcontroller and entityformcontroller to suit
Comment #22
larowlanMore fixes, but just noticed #2352155: Remove HtmlFragment/HtmlPage so going to pause this approach
Comment #24
larowlanSo before I work on those last fails - are we definitely removing HtmlFragment and the interface?
Comment #25
dawehner@larowlan
Yeah probably, so how would your solution works in case we don't have it anymore? Passing something you can alter instead of looking around doesn't seem to be much of a difference though, to be honest.
Comment #26
larowlanYep, its a shame HtmlFragment is perfect for fixing this - will read the other issue to try and get the back story on why it is up for the chop
Comment #27
larowlanThe reasoning behind removing HtmlFragment make sense.
Comment #28
catchSo I think the non-HtmlFragment of this could end up similar to larowlan's plan.
- if you don't care about #title or it can be set in the YAML or a title callback, just return the render array
- if you care about title, and it has to happen in your page's _pre_render callback, then you'll have to early-render the page array yourself with drupal_render(), and return one with #markup, all the #attached, and most importantly #title set.
Then it'd just be a case of taking $content['#title'] if it's set.
So this pushes down the responsibility of ensuring #title is set to the controller.
This adds more drupal_render() calls which is not good though, so if it's the only viable way to fix this it might end up a won't fix.
Comment #29
Fabianx CreditAttribution: Fabianx commentedSo if #title is bubbeable we store its state in the stack.
Larowlan showed its possible to store it in some object.
Why don't we just add a title service, which holds the current page title?
The last call wins - similar to how it was before ...
Yes, this is kinda global state, but so is the stack ...
--
Alternatively like #28, we could define that the #title if set, MUST BE in the top level render element or not.
That would prevent us from having to drupal_render() it, but would still allow for controllers to set a custom page title.
And if you need something else, you could implement your own service for setting a title, and then putting that in the top-most-level of the render array.
Comment #30
Fabianx CreditAttribution: Fabianx commentedAnother possibility is a conditional placeholder.
That means:
which returns whatever the current title is when not in a stack and a placeholder using #post_render_cache for all other cases.
A more complex possibility (suggested in another context by EclipseGC) would be to have a title 'context', similar to how there can be e.g. a node context.
However similar to views_get_current_view() it remains difficult to push the default page controller to a #pre_render callback.
e.g. for the case where you want to display the number of search results. This is only known once you have executed the code to do so ...
However even that code could still execute directly and return attached #title as top level of the render array.
What is important to note is that any code that uses that "global" information though is inherently bound to the page, whereas a dynamic placeholder would even work with caching.
Comment #31
larowlanDo we still need to do this now HtmlPage et al are gone?
Comment #32
Wim LeersWe would be able to get rid of this work-around if this were to happen:
It'd also be a better/more consistent DX if this patch got in.
But even if we choose to not make the necessary API change, the patch in this issue fixed many, many broken titles. If we choose not to fix the API, then we should rescope the issue to fix the many broken titles.
Comment #33
catchCould we split the broken title fixing out to another issue? That's an easier commit and then the patch with the API change will be easier to review if we want to go ahead with it.
Comment #34
dawehnerGood idea!
Created an issue for that: #2403359: Use _title and _title_callback where possible
Comment #35
Wim LeersThanks for opening that, dawehner! I made the patch in #1 (which didn't contain all the (heroic!) work by larowlan to get rid of title bubbling) apply again to HEAD and uploaded the patch to the issue you opened.
Comment #36
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedComment #37
Wim LeersI don't think this is actually a child of #2429287: [meta] Finalize the cache contexts API & DX/usage, enable a leap forward in performance — this does not block cacheability of things nor SmartCache nor BigPipe.
So, turning that parent issue into a related issue, because it definitely is related.
Comment #38
andypostI think better to keep approach with returning
#title
as top-level of buildarrayIf we going to separate title in #2476947: Convert "title" page element into a block then we need this bubbling from one block to another and also separate preprocess to build HEAD title or read it from global state (#29)
But this "state" could be a block context or request attribute, last patch passes fragment to each controller but we used to pass request already and have a stack of them.
Maybe a kind of title resolver just needs to be updated to get page title from master request atrribute?
why the last does not remove fragment from attributes?
Comment #40
Wim LeersRelated: #2705293: A page's <title> should not be determined by html.html.twig and hook_preprocess_html(), but should be #attached and bubbled up.
Comment #41
Wim LeersComment #42
dawehnerLet's replace the title to be more realistic
Comment #43
Wim Leers#42++
Thanks Daniel!
Comment #45
catchComment #46
catchComment #47
catchPostponing on #2403359: Use _title and _title_callback where possible.
Comment #48
effulgentsia CreditAttribution: effulgentsia at Acquia commentedClarifying issue title so people don't think we're trying to remove #title from form elements, etc.
Comment #49
tim.plunkett:)
Comment #50
dawehnerWell, I still believe that deprecate doesn't describe it really well, because deprecating has the tone that its gonna be removed in the future, which at the moment, doesn't work in that architecture, as described multiple times in this issue already :)
Comment #51
catchHow about that?