Problem/Motivation

To be able to cache a site you not only need the css/js attached but also the page.

Proposed resolution

Put the title to #attached and remove drupal_set_title completely.

Remaining tasks

Convert all instances of drupal_set_title to put the title to the render element.

API changes

(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate)

Sub-tasks #

Remaining

Module Issue
install / update system #2192649: Remove drupal_set_title() from installation and update process Assigned to: sun
final removal #2209595: Remove drupal_set_title(), drupal_get_title() and associated tests

Completed

Module Issue
authorize.php #2192653: Remove drupal_set_title from authorize.php
block #2102369: Remove drupal_set_title in custom block module controllers and entitylist controllers
comment #2102437: Remove drupal_set_title in comment module controllers
config #2102441: Remove drupal_set_title in config module controllers
config_test #2102443: Remove drupal_set_title in config_test module controllers
content_translation #2102445: Remove drupal_set_title in content_translation module controllers
entity #2102447: Remove drupal_set_title in entity module controllers
entity_test #1987694: Convert entity_test callbacks to a new style controller
field_ui #2102449: Remove drupal_set_title in field_ui module controllers
filter #2102451: Remove drupal_set_title in filter module controllers
help #2102453: Remove drupal_set_title in help module controllers
image #2102459: Remove drupal_set_title in Image module controllers
menu #2102461: Remove drupal_set_title in menu module controllers
node #2102465: Remove drupal_set_title in node module controllers
page cache #2209583: Remove use of drupal_set_title()/drupal_get_title()/_system_path from page cache
path #2102467: Remove drupal_set_title in path module controllers
picture #2102469: Remove drupal_set_title() in PictureMappingFormController controller and update t() with $this->t() in
shortcut #2102473: Remove drupal_set_title in shortcut module controllers Assigned to: kgoel
system/entity_test #2102481: Remove drupal_set_title in system/entity_test module controllers
system/test_page_test #2102483: Remove drupal_set_title in system/test_page_test module controllers
system/entity_test #2102481: Remove drupal_set_title in system/entity_test module controllers
system/test_page_test #2102483: Remove drupal_set_title in system/test_page_test module controllers
views #2102489: Remove drupal_set_title in views module controllers (last instance of drupal_set_title() in modules) Assigned to: dawehner
system #2102475: Remove drupal_set_title in system module controllers
system/ajax_test #2102479: Remove drupal_set_title in system/ajax_test module controllers
taxonomy #2102485: Remove drupal_set_title in taxonomy module controllers
tests and documentation #2192645: Remove drupal_set_title() from tests and documentation Assigned to: ianthomas_uk
tracker #2102487: Add back in tab removed in [#1972990] and remove drupal_set_title() in tracker module controllers
Files: 
CommentFileSizeAuthor
#30 drupal-1830588-30.patch5.79 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,238 pass(es), 5 fail(s), and 108,953 exception(s).
[ View ]
#29 drupal-1830588-29.patch6.31 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 57,333 pass(es), 5 fail(s), and 108,873 exception(s).
[ View ]
#24 1830588-set-title-23.patch700 bytesvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 58,028 pass(es), 24 fail(s), and 17 exception(s).
[ View ]
#22 1830588-set-title-22.patch1.33 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1830588-set-title-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 1830588-diff-21-22.txt449 bytesvijaycs85
#21 1830588-set-title-21.patch439 bytesvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,291 pass(es).
[ View ]
#21 title-alter.png63.38 KBvijaycs85

Comments

dawehner’s picture

So one point is that if you put the title on the element which is returned on a page you have to find a way how the attached title is propagated from that element to the final

array(
  '#theme' => 'page',
  '#attached' => array(
    'title' => ..
  ),
)
catch’s picture

Category:feature» task
Priority:Normal» Major
sun’s picture

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 (title, tabs, actions) into blocks
#1222248: Remove theme_book_title_link() use l() instead

tim.plunkett’s picture

Issue tags:+VDC

Tagging.

catch’s picture

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.

moshe weitzman’s picture

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.

David_Rothstein’s picture

Priority:Major» Normal

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

catch’s picture

Title:Use #attached title to store the title» Put the page title on the page array (or its replacement), remove drupal_set_title()
Priority:Normal» Major

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.

sun’s picture

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.

David_Rothstein’s picture

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.

David_Rothstein’s picture

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

YesCT’s picture

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.

LittleCoding’s picture

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

effulgentsia’s picture

Issue tags:+caching

Adding the caching tag to ESI related issues.

xjm’s picture

Issue tags:-caching+D8 cacheability
catch’s picture

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

dawehner’s picture

Issue tags:+scotch

Adding the probably most important tag here.

larowlan’s picture

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

xjm’s picture

Issue tags:-scotch+Blocks-Layouts

Actual tag for SCOTCH. :)

Crell’s picture

Issue tags:+WSCCI

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

vijaycs85’s picture

Status:Active» Needs review
StatusFileSize
new63.38 KB
new439 bytes
PASSED: [[SimpleTest]]: [MySQL] 58,291 pass(es).
[ View ]

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:
title-alter.png

vijaycs85’s picture

StatusFileSize
new449 bytes
new1.33 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1830588-set-title-22.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1830588-set-title-22.patch, failed testing.

vijaycs85’s picture

Status:Needs work» Needs review
StatusFileSize
new700 bytes
FAILED: [[SimpleTest]]: [MySQL] 58,028 pass(es), 24 fail(s), and 17 exception(s).
[ View ]

Adding output flag...

Status:Needs review» Needs work

The last submitted patch, 1830588-set-title-23.patch, failed testing.

dawehner’s picture

Issue tags:+Needs tests

.

ParisLiakos’s picture

One more tag

ParisLiakos’s picture

Issue tags:+Needs tests

sigh

dawehner’s picture

Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new6.31 KB
FAILED: [[SimpleTest]]: [MySQL] 57,333 pass(es), 5 fail(s), and 108,873 exception(s).
[ View ]

Talked with katbailey about it and there is the following problem. This is the typical flow of a page in D8 now:

HttpKernel
HtmlPageController
HttpKernel->forward (which creates a subrequest)
YourController
ViewSubscriber->onView (which then renders the content of the subrequest).

<?php
     
// This is a new-style Symfony-esque subrequest, which means we assume
      // the body is not supposed to be a complete page but just a page
      // fragment.
     
$page_result = $event->getControllerResult();
      if (!
is_array($page_result)) {
       
$page_result = array(
         
'#markup' => $page_result,
        );
      }
     
$event->setResponse(new Response(drupal_render($page_result)));
?>

The problem is onView as it renders the content, which doesn't allow to get any information out of the subrequest to the main request beside the content. To sum it up, there is no proper way to return something else then the rendered result.

dawehner’s picture

StatusFileSize
new5.79 KB
FAILED: [[SimpleTest]]: [MySQL] 57,238 pass(es), 5 fail(s), and 108,953 exception(s).
[ View ]

Let's not kill the tests.

Status:Needs review» Needs work

The last submitted patch, drupal-1830588-30.patch, failed testing.

catch’s picture

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

moshe weitzman’s picture

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.

catch’s picture

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

dawehner’s picture

@moshe
Let's think of a ESI world, where every block and also the content of the page is a subrequest, which is supposed to render a response containing the rendered output. You can't defer the rendering until the very end anymore.

moshe weitzman’s picture

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.

Crell’s picture

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

thedavidmeister’s picture

@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()?

moshe weitzman’s picture

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

thedavidmeister’s picture

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

catch’s picture

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

catch’s picture

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

moshe weitzman’s picture

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

dawehner’s picture

pwolanin’s picture

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?

katbailey’s picture

Assigned:Unassigned» katbailey

Taking a shot at this.

dawehner’s picture

#2032535: Resolve 'title' using the route and render array at least also allows you to use the render array to specify a title.

katbailey’s picture

Assigned:katbailey» Unassigned

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.

dawehner’s picture

Let's mark it as duplicate, of #2032535: Resolve 'title' using the route and render array just to be sure.

xjm’s picture

Title:Put the page title on the page array (or its replacement), remove drupal_set_title()» [META] remove drupal_set_title()
Priority:Major» Critical
Status:Needs work» Postponed
Issue tags:-major and critical issue threshold sweep

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.

xjm’s picture

Issue tags:+API change

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

catch’s picture

Issue tags:+Approved API change

Yep.

dawehner’s picture

The next step for this issue is for sure to convert all instances of drupal_set_title to use the return array.

jibran’s picture

vijaycs85’s picture

Issue summary:View changes

Adding sub-tasks

vijaycs85’s picture

Issue summary:View changes

Updated issue summary with sub-task list

vijaycs85’s picture

Issue summary:View changes

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

vijaycs85’s picture

Issue summary:View changes

Updated issue summary.

xjm’s picture

Issue summary:View changes
Issue tags:+beta blocker
vijaycs85’s picture

Issue summary:View changes

Cleaning up sub-tasks

vijaycs85’s picture

Issue summary:View changes
xjm’s picture

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.

MustangGB’s picture

Issue summary:View changes
MustangGB’s picture

Issue summary:View changes
webchick’s picture

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?

webchick’s picture

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');
Crell’s picture

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

xjm’s picture

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

thedavidmeister’s picture

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.

ianthomas_uk’s picture

Issue summary:View changes

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

cosmicdreams’s picture

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.

thedavidmeister’s picture

@cosmicdreams - go ahead. Seems logical to me.

ianthomas_uk’s picture

Issue summary:View changes

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

ianthomas_uk’s picture

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.

YesCT’s picture

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

dawehner’s picture

Yeah drupal_get_title() should also be removed.

martin107’s picture

refreshing stale status of links in summary

penyaskito’s picture

Title:[META] remove drupal_set_title()» [META] remove drupal_set_title() and drupal_get_title()

Retitling per latest comments.

ianthomas_uk’s picture

Issue summary:View changes

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.

ianthomas_uk’s picture

ianthomas_uk’s picture

Issue summary:View changes
webchick’s picture

Status:Active» Fixed

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

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xtfer’s picture

Status:Closed (fixed)» Needs work
Issue tags:+Needs change notification

This needs a change record for drupal_get_title().

xtfer’s picture

Priority:Critical» Normal
xtfer’s picture

Draft change notification created.

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

ianthomas_uk’s picture

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?

tim.plunkett’s picture

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.

xjm’s picture

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

xjm’s picture

Issue tags:-Needs change notification+Missing change record

Fixing tag.

dawehner’s picture

Status:Needs work» Fixed

Both drupal_set_title and drupal_get_title() is finally gone.

xjm’s picture

Title:[META] remove drupal_set_title() and drupal_get_title()» Change record: [META] remove drupal_set_title() and drupal_get_title()

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

xjm’s picture

Status:Fixed» Active
dawehner’s picture

Yeah noone is writing change records ever: https://drupal.org/node/2067859

dawehner’s picture

Just in case you read this: No there is no alternative to drupal_get_title() as there is only one code path which actually needs it

dawehner’s picture

If someone really wants to write one for drupal_get_title() I really think it is pointless.

tim.plunkett’s picture

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

dawehner’s picture

Status:Active» Fixed

good

Gaelan’s picture

Title:Change record: [META] remove drupal_set_title() and drupal_get_title()» [META] remove drupal_set_title() and drupal_get_title()
Priority:Normal» Critical
Issue tags:-Missing change record

Merged the two change records.

ianthomas_uk’s picture

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.

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.