[META] remove drupal_set_title() and drupal_get_title()

#1833342: Use #attached (or similar) for the page title was duplicate.

I don't think that #attached is the right architectural answer.

1. A page consists of many rendered elements, but only one, and only one page title only.
2. There may be multiple render elements that have an #attached title.
3. You cannot control the order in which elements are rendered.
4. This is, in fact, incompatible with parallel rendering.
5. Currently, we're doing this at the controller level (in new router speak) [page callback in old speak], which sorta guarantees to some extent that only one page title gets set.

If we were to pursue the idea of #attached, then I think we would have to completely change the paradigm in which we're rendering "blocks" as well as the main page content:

The main page content would have to be treated like a block, and every block's title is #attached to the block. This means that the $title variable in page.tpl.php would cease to exist.

This idea was originally discussed for #1077602-93: Convert node.tpl.php to HTML5, but got deferred to a follow-up issue:

#1328048: Page title location needs to be flexible

We further have to differentiate between a block title and the HTML page title, as these are two different can of worms.

Every block has a #title, but only one block builds the base for the HTML page title (but is not necessarily the same). Furthermore, a block #title supports HTML, whereas the HTML page title does not.

Closely related:
#465958: Allow to define separate 'page title' in hook_menu()
#507488: Convert page elements (local tasks, actions) into blocks
#1222248: Remove theme_book_title_link() use l() instead

Tagging.

Yes #attached won't work with parallel rendering, but drupal_set_title() doesn't work with it either - you can't guarantee that your code that sets the page title will run last except for the arms race of picking hooks and module/hook weights.

I'd be completely fine with removing the ability for arbitrary code to set the page title - it could be something attached to the Scotch display controller thingies (or the page-level render array now) and you only get to set it there, nowhere else. Similar for block titles.

Yeah, maybe what we do for now is start expecting a #title at top level of $page array and we convert existing calls to drupal_set_title() to hook_page_build() implementations. Similarly, calls to drupal_add_js, drupal_add_css, drupal_set_breadcrumb() should move to hook_page_build() and set #attached on top level. We can do it differently once SCOTCH exists.

This is an important issue, but I don't believe it's a major task (see http://drupal.org/node/45111 and http://drupal.org/node/1181250). Specifically, although render caching is important, computing the page title is rarely that expensive (maybe at most involving a single entity load which can itself be cached?) so if you can put large parts of the page in the render cache but not the page title, you're still in pretty good shape.

If you disagree and feel this must still be marked major, please provide a very specific justification and keep in mind that every major issue prevents other features that people are working hard on from getting into Drupal core, due to the issue queue threshold policy (until/unless #1810428: [Policy, no patch] Adjust major and critical task thresholds during code thaw is addressed, at any rate). Note that I am not singling out this issue for particular attention but rather (slowly) trying to go through the issue queue and do this in many places so that other features still have a chance to get into Drupal core soon without the thresholds getting in the way. I am trying my best to be impartial, and of course, just because an issue isn't marked "major" certainly doesn't mean it's unimportant to work on. You can follow my progress via the tag I've just added to this issue :)

This isn't about caching the title, it's about ensuring the title is the same regardless of cache settings.

If I have code in a block that calls drupal_set_title(), and the block is cached, then my code will only run on cache misses and the title itself will be unpredictable. This makes it impossible to cache rendered output and reliably have a working site. So moving back to major for that reason. I agree that just caching the page title would not be major at all.

I think moshe's idea is the better one here and would also work for breadcrumbs, fixes sun's concern at least, so changing issue title to reflect that.

I'm not able to understand the proposed change. A "page callback" block does not have access to the $page render array, by design.

Introducing that knowledge would be a step in a wrong direction.

The only remotely possible way I could imagine would be to bake the logic into the whichever_function_it_is_that_calls_the_page_callback_today() function, and from there, dynamically call drupal_set_title($build['#title']).

That's an entirely different spot in the code than $page though.

OK, fair enough, although I think any code (right now at least?) that tries to set the title from inside a non-page block callback is probably doing something on the edge of what Drupal supports anyway... so this mostly seems relevant for caching the page callback.

I kind of agree with @sun that this belongs in the page callback itself somewhere. The information you need to set a dynamic page title is often already passed directly in the page callback function, but more complicated to access in hook_page_build(), so it seems like $build['#title'] in the page callback function is simpler. As long as there is some alter hook that allows others to change it (hook_block_view_alter(), I guess?) it should be just as good as hook_page_build()/hook_page_alter() in terms of alterability also.

Sorry for the noise; I'm told that slashes in tags can break the autocomplete on drupal.org.

Looks like some agreement on the proposed solution

Next steps:
either: a patch to get things rolling
or detailed/clarified/summized next steps for what such a patch should do

so, the issue summary can probably be updated with that info.

Would the work going on to have core work better with the sectioning requirements of HTML5 also fix this issue?

#1328048: Page title location needs to be flexible

Adding the caching tag to ESI related issues.

Whichever approach we end up doing here will also need to happen for breadcrumbs.

Does moving the title output to a block with cache_per_page fix this issue?

Actual tag for SCOTCH. :)

No wonder I didn't know this was here...

Hope, I understand the approach from the comments above. Here is the first page and in action in dblog.module:

diff --git a/core/modules/dblog/dblog.module b/core/modules/dblog/dblog.module
index 202cfcf..01819a0 100644
--- a/core/modules/dblog/dblog.module
+++ b/core/modules/dblog/dblog.module
@@ -88,6 +88,7 @@ function dblog_menu() {
  */
 function dblog_page_build(&$page) {
   if (arg(0) == 'admin' && arg(1) == 'reports') {
+    $page['#title'] = 'Reports title alter';
     $page['#attached']['css'][] = drupal_get_path('module', 'dblog') . '/css/db
   }
 }

and got this:

As suggested by @timplunkett on IRC changing #title => #page_title

Adding output flag...

One more tag

sigh

$event->setResponse(new Response(drupal_render($page_result)));

Right, this is relying on the fact that drupal_render() calls drupal_add_js() etc. to put assets and similar into a static.

While I think we have issues open for everything that directly puts things into a static, I don't think there's one for drupal_render() itself yet - so a direct call to drupal_render() anywhere before final page building still scuppers this.

sdboyer was working on a 'page fragment' class which would encapsulate both content and assets which I assume is in the princess branch somewhere but haven't reviewed that yet.

Probably needs a new major/critical issue for this.

Well, the guideline since D7 has been that you don't call drupal_render(). You are supposed to return render arrays from blocks and page callbacks and then kick off all rendering at the end with drupal_render_page(). I notice that we have been adding calls to drupal_render() instead of removing them in issues like #2012812: drupal_render() can't distinguish between empty strings from theme() and no hook being matched. Lets try not muddy the render pipeline with more calls like that. We have to figure out a way to handle nested theme() calls without using drupal_render(). Or disallow nested theme() calls.

Drupal 7 contains 88 calls to drupal_render() many from theme functions - this was never enforced or completed so we need to finish that here. There's a lot of those theme() -> drupal_render() issues around at the moment which to me felt more consistent than using theme() sometimes and drupal_render() others, but yes it's making this particular problem worse (although the theme() calls wouldn't handle assets correctly either of course).

Whatever is calling those subrequests should perform the rendering. Those blocks have to return their list of js/css/library in addition to a string. Blocks can always return this structure and I don't see how early render helps.

Crazy idea:

https://drupal.org/node/2026025

That's how breadcrumbs work now, making them entirely deterministic. Might something similar work for the page title? (Possibly with some default implementations that pull data from the route?)

@moshe, did you link the right issue? the one you linked is just trying to address a bug in drupal_render(), it doesn't change where it is being called, did you mean #2006152: [meta] Don't call theme() directly anywhere outside drupal_render()?

@thedavidmeister - yes, i meant to link to that issue. thanks.

@Crell - I don't think thats an ideal solution, for title (nor do I love the new breadcrumbs solution). I prefer the #attached['page_title'] approach as mentioned in the Summary.

Consider a View that has a custom page title. The View gets executed at some point during page build. If we move to a page title to a Service like breadcrumbs, then the Views implementation would presumably access a statically cached View to calculate and return the title. Thats OK, until we do some block caching and the View never gets executed. The Page title service would then have to execute the View in order to return a proper title. Our caching has become useless.

This comes back to PageFragment or whatever Scotch calls it. Until thats real, I think render array and #attached is a good and consistent approach.

@moshe, just to clarify, the conversions in that issue are not rendering anything earlier than they were previously despite the increase in the number of drupal_render() calls.

I'd +1 any meta issue that audited drupal_render() calls and started weeding out ones that looked too early so the total number of drupal_render() calls is lowered overall - I'd be willing to help with that.

We have to figure out a way to handle nested theme() calls without using drupal_render(). Or disallow nested theme() calls.

The meta issue that I linked is basically the latter, or at least it enables enforcing the latter.

@moshe, for views, if that's the main page callback and it returns the title it wouldn't need a service. It'd be the same as #attached except only the main page callback gets to have that processed. What we can't do is have any block returning #attached since there's no way to pick - in this way it's different from assets where any number of blocks can add any number of assets.

@dawhener while you can't rely on late rendering, you also can't rely on anything being put into a static. If 'real' sub-requests are being made as a result of ESI/CSI etc. then they'll need a way to return that information to the document being served - could be rel links for RSS, CSS or JavaScript - anything that needs to go in <head> (or scripts in the footer etc.).

That's not implemented anywhere and doesn't have to be in core, but regardless of how it eventually gets done, direct calls to drupal_render() outside of the specific flow will break it (because the asset collection happens inside drupal_render() with statics and you only get a string back). Fixing this for render caching fixes it for *SI too, the only difference is the mechanism whereby the #attached stuff makes it to the final rendered page, but the problem collecting it is the same.

@guys -lets not discuss "we can't do late rendering anymore". i understand that, and i'm mostly ok with it.

What we can't do is have any block returning #attached since there's no way to pick - in this way it's different from assets where any number of blocks can add any number of assets.

The traditional way to solve that is the #weight system. Seems like we could support this key within #attached. And we can always have a drupal_alter or equivalent for custom code to reorder as needed.

In regards to #1981644: Figure out how to deal with 'title/title callback', dawehner is suggesting putting a default string title (or method to get the title) on the route. Perhaps we should move that piece here, since the output of that is what should be populating this page element?

Taking a shot at this.

Not sure if this should be marked as a duplicate or if we should keep it for removing drupal_set_title() entirely. Submitted a patch over in #2032535-5: Resolve 'title' using the route and render array for adding the #title property to the render array.

Related #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead

Discussed with @dawehner -- we still need to remove the function, but multiple conversions would be required first. So, scoping as a meta and postponing on #2032535: Resolve 'title' using the route and render array.

Also, I believe this is now release-blocking as part of #1839338: [meta] Remove drupal_set_*() drupal_add_*() in favour of #attached/response for out-of-band stuff.

Also, removing that function is obviously an API change. We should have a core maintainer approve this specifically.

Yep.

#2032535: Resolve 'title' using the route and render array is in.

Adding sub-tasks

Updated issue summary with sub-task list

Updated issue summary - update for # in sub task section.

Updated issue summary.

Cleaning up sub-tasks

Looking great here. What's outstanding? I see that #2102485: Remove drupal_set_title in taxonomy module controllers is postponed on #1857256: Convert the taxonomy listing and feed at /taxonomy/term/%term to Views, which is great to get in on its own merits.

I grepped and found the following other calls to it:

core/authcore/authorize.php
core/includes/authorize.inc
core/includes/batch.inc
core/includes/bootstrap.inc
core/includes/errors.inc
core/includes/install.core.inc
core/lib/Drupal/Core/Controller/ExceptionController.php
core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php
core/modules/content_translation/content_translation.pages.inc
core/modules/node/lib/Drupal/node/NodeFormController.php
core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php
core/modules/system/system.api.php
core/modules/system/tests/modules/entity_test/entity_test.module
core/modules/views_ui/lib/Drupal/views_ui/ViewUI.php
core/update.php

(Edited to be a legible list of files.) So bootstrap.inc is last because that's the function itself. We have a bunch of work to do in components, though, which might be a bit more gnarly than in modules. Plus the installer and update.php.

I believe the last of these module-wise, #2102489: Remove drupal_set_title in views module controllers (last instance of drupal_set_title() in modules) just got committed. How about we make a patch here for the rest that includes the instances in #59 and whatever else we've missed since then?

Here's what grep is showing me as of right now:

./core/authorize.php:  drupal_set_title('Access denied');
./core/authorize.php:    drupal_set_title($_SESSION['authorize_operation']['page_title']);
./core/authorize.php:    drupal_set_title(t('Authorize file system changes'));
./core/authorize.php:      drupal_set_title($results['page_title']);
./core/includes/authorize.inc:    drupal_set_title($operation['page_title']);
./core/includes/batch.inc:  drupal_set_title($current_set['title'], PASS_THROUGH);
./core/includes/bootstrap.inc: * Flag for drupal_set_title(); text has already been sanitized.
./core/includes/bootstrap.inc:  $title = drupal_set_title();
./core/includes/bootstrap.inc:function drupal_set_title($title = NULL, $output = Title::CHECK_PLAIN) {
./core/includes/bootstrap.inc:      drupal_set_title($cache->data['title'], PASS_THROUGH);
./core/includes/errors.inc:      drupal_set_title('Error');
./core/includes/install.core.inc: * Installation tasks should use drupal_set_title() to set the desired page
./core/includes/install.core.inc:  drupal_set_title(t('Database configuration'));
./core/includes/install.core.inc:        drupal_set_title(t('Select an installation profile'));
./core/includes/install.core.inc:      drupal_set_title(t('Choose language'));
./core/includes/install.core.inc:  drupal_set_title(t('No profiles available'));
./core/includes/install.core.inc:  drupal_set_title(t('Drupal already installed'));
./core/includes/install.core.inc:  drupal_set_title(t('Configure site'));
./core/includes/install.core.inc:  drupal_set_title(t('@drupal installation complete', array('@drupal' => drupal_install_profile_distribution_name())), PASS_THROUGH);
./core/includes/install.core.inc:      drupal_set_title(t('Requirements problem'));
./core/lib/Drupal/Core/EventSubscriber/MaintenanceModeSubscriber.php:      drupal_set_title(t('Site under maintenance'));
./core/lib/Drupal/Core/Page/DefaultHtmlPageRenderer.php:      // drupal_set_title() already ensured security, so not letting the
./core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php:      'description' => 'Tests correct handling or conversion by drupal_set_title() and drupal_get_title() and checks the correct escaping of site name and slogan.',
./core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php:    drupal_set_title($this->saved_title, PASS_THROUGH);
./core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php:   * Tests the handling of HTML by drupal_set_title() and drupal_get_title()
./core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php:    // drupal_set_title's $filter is Title::CHECK_PLAIN by default, so the title should be
./core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php:    drupal_set_title($title, Title::CHECK_PLAIN);
./core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php:    // drupal_set_title's $filter is passed as PASS_THROUGH, so the title should be
./core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php:    drupal_set_title($title, PASS_THROUGH);
./core/modules/system/lib/Drupal/system/Tests/System/PageTitleTest.php:    // Adding </title> so we do not test the escaped version from drupal_set_title().
./core/modules/system/system.api.php: * handled differently). You should also use drupal_set_title() within the task
./core/modules/system/tests/modules/entity_test/entity_test.module:  drupal_set_title(t('Create an @type', array('@type' => $entity_type)));
./core/modules/system/tests/modules/entity_test/entity_test.module:  drupal_set_title($entity->label(), PASS_THROUGH);
./core/update.php:  drupal_set_title('Drupal database update');
./core/update.php:  drupal_set_title('Drupal database update');
./core/update.php:  drupal_set_title('Drupal database update');
./core/update.php:  drupal_set_title('Access denied');
./core/update.php:    drupal_set_title('Requirements problem');

Looking over the list in #63, it looks like they all fall into one of a few categories:

1) The install/update system. FML.
2) An alternate front-controller, like authorize.php. We've known that we need to do something about those for a while, but haven't gotten to them yet. They all ought to just have an alternate kernel that's hard coded to what they do.
3) Tests. These SHOULD just be a matter of changing what the tests expect.
4) Comments (easy)

That's roughly in descending order of how annoying they will be to fix. Do we want one issue for each of those clusters? (Comments we can save for the final removal of drupal_set_title() itself.)

@Crell, that sounds like a good breakdown to me.

Please review #2072647: #theme 'maintenance_page' should support render arrays in #content to allow us to use renderable arrays in the install/update system.

IIRC it is blocking (1) in #64.

#2072639: Convert update_info_page() from a string concatenation function to a render array builder and #2060441: Convert update_results_page() to return a renderable array rather than early render both refer to a functions with a drupal_set_title() - they are both blocked on the linked issue in #66.

Please track/add conversions to renderable arrays at #2046881: [meta] Avoid "early rendered" strings where beneficial to do so, build structured data to pass to drupal_render() once instead.

Updating summary to show #2102485: Remove drupal_set_title in taxonomy module controllers is fixed (it was done in another issue).

Should we create issues for the ones that remain? I found an easy one to fix in Drupal\Core\EventSubscriber\MaintenanceModeSubscriber. Given that this is a meta issue I wouldn't be prudent to contribute the patch here. I could simply create an issue to catch all the remaining drupal_set_titles for each of the categories that Crell has described.

@cosmicdreams - go ahead. Seems logical to me.

I've filed three sub-issues to cover the remaining issues. I'll take tests and documentation.

#2192649: Remove drupal_set_title() from installation and update process (includes Drupal\Core\EventSubscriber\MaintenanceModeSubscriber from #69)
#2192653: Remove drupal_set_title from authorize.php
#2192645: Remove drupal_set_title() from tests and documentation

Should this cover drupal_get_title() too? Does that even work without drupal_set_title()? I imagine the title being looked for is probably still sitting in an un-rendered array if drupal_set_title() hasn't been called.

during irc conversation, @ianthomas_uk and I noticed we can't remove drupal_set_title() without removing/changing drupal_get_title() since

function drupal_get_title() {
  return drupal_set_title() ?: '';
}

in bootstrap.inc

refreshing stale status of links in summary

Retitling per latest comments.

Reorganised the remaining changes a little, including opening #2209583: Remove use of drupal_set_title()/drupal_get_title()/_system_path from page cache to ensure that change gets reviewed by someone who knows the page cache well.

#2209595: Remove drupal_set_title(), drupal_get_title() and associated tests just went in, which makes this one FIXED. Great work, all!

This needs a change record for drupal_get_title().

Draft change notification created.

Can someone with knowledge of the issue please review, update if required, and resolve?

I think it would be better to expand the drupal_set_title change record to cover this, because if you're looking at one there's a good chance you'll be interested in the other.

That D8 code is horrible. Is that really the best we have?

What are the common use case of these functions? e.g. how can I append a string to an existing, unknown title? Can I get the title from a render array?

I think its fine to keep them separate, but they can cross-link...

#2103301: Add a helper class to introspect a given request will hopefully clean up that code a bit more.

I'd actually agree with merging it into the drupal_set_title() change record, because otherwise people look at this and go "um what the HELL?"

Fixing tag.

This is open for a missing change record; see above.

That was already done: https://drupal.org/node/2231127
It's just not published

Merged the two change records.

The drupal_get_title() section was still written as if the reader had no idea drupal_set_title() had changed, and therefore duplicated the earlier documentation. I've cleaned that up. Otherwise the CR looks good.