We are very close to removing all uses of drupal_set_title() and drupal_get_title() from core.

One remaining use is drupal_page_set_cache(), but if drupal_get_title() always returns an empty string then there is no point storing that in the cache. This is a small patch, but I wanted the change to be explictly approved by the people who know this code well.

CommentFileSizeAuthor
#3 drupal-2209583-set_title-page-cache-3.patch1.29 KBianthomas_uk
#1 drupal-2209583-set_title-page-cache-1.patch1.14 KBianthomas_uk

Comments

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
StatusFileSize
new drupal-2209583-set_title-page-cache-1.patch1.14 KB
znerol’s picture

znerol commented

If you are touching that area, please also remove path. Quoting #2177461-11: Refactor page caching into a service:

The path and title keys on the cache object were introduced in order to allow the statistics module to operate for cached pages (#692044: A site with statistics module enabled is much slower in serving cached pages in D7 than in D6). Meanwhile the statistics module switched to Ajax and boot/exit-hooks were removed. Therefore it is save to also get rid of this metadata.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
Title: Remove use of drupal_set_title/drupal_get_title from page cache » Remove use of drupal_set_title/drupal_get_title/_system_path from page cache
Status: Active » Needs review
StatusFileSize
new drupal-2209583-set_title-page-cache-3.patch1.29 KB

That sounds sensible. Are you confident nothing else is using path now that it's there?

znerol’s picture

znerol commented
Issue tags: +Quick fix

Are you confident nothing else is using path now that it's there?

I do not see anything in core relying on the presence neither of title nor of path.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Reviewed & tested by the community

Those most likely used to be useful, but it is clear that they are already broken in Drupal 8 (because they rely on static storage of those that we already broke), so +1 on removing them completely.

ianthomas_uk’s picture

ianthomas_uk
he/him
Primary language English
Location Brighton, UK
commented
dawehner’s picture

dawehner
Primary language German
commented

Wait: don't we want to used #title and store it? Just because it is broken does not mean we should not try to do it right.

damien tournoud’s picture

damien tournoud commented

@dawehner: doing it right means refactoring the page cache completely, which is out of the scope of this issue. This is just removing code that cannot work anyway.

znerol’s picture

znerol commented

In my opinion any page level cache architecture relying on metadata which is not present on the HTTP layer is simply broken.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Remove use of drupal_set_title/drupal_get_title/_system_path from page cache » Remove use of drupal_set_title()/drupal_get_title()/_system_path from page cache

Manually tested, RTBC++

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

dawehner’s picture

dawehner
Primary language German
commented

@dawehner: doing it right means refactoring the page cache completely, which is out of the scope of this issue. This is just removing code that cannot work anyway.

Sure, so should we either add a todo or a follow up to ensure that the bit don't get lost completly?

Status: Fixed » Closed (fixed)

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