The drupal_render_cache_generate_placeholder() requests a specific unique token and is output as a 'token' attribute in the placeholder HTML. The problem I see with this is that all the usages in core also pass in the same token in $context['token'] so it gets embedded via the context array. This results in duplication because we're always doing drupal_render_cache_generate_placeholder($callback, $context, $context['token']) and it duplicates the token value in the context and token attributes of the placeholder HTML.

I propose that drupal_render_cache_generate_placeholder() removes the 'token' argument and provides one by default by merging it into the $context array if not already present.

CommentFileSizeAuthor
#9 interdiff.txt693 bytesWim Leers
#9 2275679-render-cache-token-unnecessary-9.patch12.96 KBWim Leers
#7 2275679-render-cache-token-unnecessary.patch12.96 KBDave Reid
#6 interdiff.txt3.27 KBDave Reid
#6 2275679-render-cache-token-unnecessary.patch12.93 KBDave Reid
#4 2275679-render-cache-token-unnecessary.patch9.92 KBDave Reid
#2 2275679-render-cache-token-unnecessary.patch9.64 KBDave Reid
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Issue summary: View changes
Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
2275679-render-cache-token-unnecessary.patch9.64 KB

Confirmed that removing drupal_render_cache_generate_token() works just fine and makes all the code that uses render cache placeholders a bit simpler since they're all doing exactly the same thing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Title: post_render_cache token does not seem to ever be used » Clean up render cache placeholder
Issue tags: +D8 cacheability

YES! Thanks for creating this issue!

This is a consequence of changes made in #2099131: Use #pre_render pattern for entity render caching, we failed to clean it up there.

So +1 to:

-  return '<drupal:render-cache-placeholder callback="' . $callback . '" context="' . $context_attribute . '" token="' . $token . '" />';

The only thing I'm not sure of is whether we want

+  return '<drupal:render-cache-placeholder callback="' . $callback . '" context="' . $context_attribute . '" />';

or:

+  return '<drupal:render-cache-placeholder callback="' . $callback . '" token="' . $token . '" />';

or even:

+  return '<drupal:render-cache-placeholder token="' . $token . '" />';

I think it's useful to have the callback serialized in there for debugging purposes, but the entire context… that's less useful.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Active » Needs review
FileSize
2275679-render-cache-token-unnecessary.patch9.92 KB

Yeah I will agree having the callback in the token was more helpful at times than having the context in there. I debated going down to just the token attribute, but let's leave callback in there.

Status: Needs review » Needs work

The last submitted patch, 4: 2275679-render-cache-token-unnecessary.patch, failed testing.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Status: Needs work » Needs review
FileSize
2275679-render-cache-token-unnecessary.patch12.93 KB
interdiff.txt3.27 KB

Fixed test failures.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
FileSize
2275679-render-cache-token-unnecessary.patch12.96 KB

Sigh, I accidentally removed the callback attribute from the placeholder.

The last submitted patch, 6: 2275679-render-cache-token-unnecessary.patch, failed testing.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented
Status: Needs review » Reviewed & tested by the community
FileSize
2275679-render-cache-token-unnecessary-9.patch12.96 KB
interdiff.txt693 bytes

#7 is perfect, except for one stupid typo. So fixing that typo and RTBC'ing at the same time.

Thanks a lot, Dave! :)

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Yes, thanks for fixing my inability to spell uniquely. Sigh.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

  • Commit 49be354 on 8.x by catch: 
    Issue #2275679 by Dave Reid, Wim Leers: Clean up render cache...

Status: Fixed » Closed (fixed)

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

Fabianx’s picture

Fabianx CreditAttribution: Fabianx commented
Status: Closed (fixed) » Needs work

The original change notice for this, that I can't even seem to find atm. needs an update. It has the $context token example wrong.

Also the documentation https://api.drupal.org/api/drupal/core!includes!common.inc/function/drup... is wrong and still speaks of drupal_pre_render_render_cache_placeholder.

And another bug:

-  return '<drupal-render-cache-placeholder callback="' . $callback . '" token="' . $context['token'] . '"></drupal-render-cache-placeholder>';
+  return '<drupal-render-cache-placeholder callback="' . $callable . '" token="' . $context['token'] . '"></drupal-render-cache-placeholder>';

else the whole setup in the beginning is in vain ...

Edit: Not a bug, just a check that the $callback is callable.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

  • catch committed 49be354 on 8.3.x 
    Issue #2275679 by Dave Reid, Wim Leers: Clean up render cache...

  • catch committed 49be354 on 8.3.x 
    Issue #2275679 by Dave Reid, Wim Leers: Clean up render cache...
Fabianx’s picture

Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commented
Status: Needs work » Closed (fixed)

This functionality has been replaced with normal placeholders => closing again.