- Issue created by @longwave
- Merge request !383Resolve #3484671 "Add support for block derivatives" β (Merged) created by longwave
- π¬π§United Kingdom longwave UK
Does not look like this is too tricky, except for the FullyValidatable requirement, which is not defined for almost any block in 10.x.
- π¬π§United Kingdom longwave UK
Menus and views blocks are available in the UI, menus have some basic test coverage.
- π¬π§United Kingdom longwave UK
Any clues as to where max-age 1 instead of -1 is coming from are welcome, so far I've been unable to track it down.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
#6 I bet that it's a
TRUE
being cast to1
.I bet you already tried putting a breakpoint in
\Drupal\Core\Cache\Cache::mergeMaxAges()
?#3:
[β¦] except for the FullyValidatable requirement, which is not defined for almost any block in 10.x.
Correct, π Make Block config entities fully validatable Fixed only landed in Drupal
11.1.x
!I think that doesn't need to be a problem though:
- On Drupal
^10 | ^11.0
, zero block plugins areFullyValidatable
; but any block plugins that have no settings do not need any validation. So, on those Drupal versions, only such blocks would be available in XB. - On Drupal
^11.1
, a few block plugins are fully validatable, so on those versions, more block plugins will appear in XB π
IOW: version-specific test expectations, and to get the full power of XB, you have to use a sufficiently rich version of Drupal core.
- On Drupal
- π¬π§United Kingdom longwave UK
I've spent several hours figuring what's going on here with max-age, writing this up quickly so I don't forget where I am.
This only happens from inside the test runner, I can't reproduce it from outside with cache debugging headers turned on.
The culprit is the
views_block:content_recent-block_1
block plugin. Deep inside Views,$view->element['#cache']['max-age']
gets changed from -1 to 1, seemingly inStylePluginBase::renderFields()
here:// Views may be rendered both inside and outside a render context: // - HTML views are rendered inside a render context: then we want to // use ::render(), so that attachments and cacheability are bubbled. // - non-HTML views are rendered outside a render context: then we // want to use ::renderInIsolation(), so that no bubbling happens if ($renderer->hasRenderContext()) { $renderer->render($data); } else { $renderer->renderInIsolation($data); }
Because we mess with render contexts ourselves in the ApiComponentsController, I am guessing something goes wrong here with bubbling the cache metadata.
Next up: investigate what happens inside the renderer when this happens to see why max-age gets set to 1.
- π¬π§United Kingdom longwave UK
Finally tracked it down. It's the
changed
field that is rendered in the Recent Content block. This uses the "timestamp_ago" formatter. In the test, the recent content was only just created, so the granularity of the timestamp is in seconds, and in turn the max-age of the field is set to 1 second inDateFormatter::formatDiff()
:case 's': $max_age = min($max_age, 1); break;
The max-age then bubbles all the way up to the components API response.
The reason I couldn't reproduce this the same way outside of the test is that my recent content is not quite so recent! There is a fallback in ApiComponentsController:
// Even if one or more of the component previews is technically not // cacheable, cache it anyway for up to 1 minute, precisely because it is // just a preview. if ($cacheability->getCacheMaxAge() === 0) { $cacheability->setCacheMaxAge(60); }
So the fix would be to adjust this to set it to 60 if it's less than 60 (but not permanent/-1), instead of only if it's 0.
- π¬π§United Kingdom longwave UK
Also discovered while working on this: blocks can vary, including per-user. For example, you can have a Views block with contextual filter that defaults to the current user. This means that the previews effectively become uncacheable, if the
user
cache context is bubbled up to the response. If we ignore this, we risk leaking information about the user that happened to store the cache to other users.For now, I've worked around this by explicitly switching to the anonymous user during rendering and then ignoring any user-related cache contexts. But this is probably not a long term solution; we should decide what is acceptable here. Maybe a better answer is to cache each preview separately, then we can vary per-user blocks without having to re-render static components.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
#8:
I've spent several hours figuring what's going on here with max-age, writing this up quickly so I don't forget where I am.
This only happens from inside the test runner, I can't reproduce it from outside with cache debugging headers turned on.
π€― That's worrying! Been a long time since I encountered a Heisenfailure π
#9:
Finally tracked it down. @Wim unfortunately you lost that bet!
π
#9: wow, that is β¦ just wow. +1 to your proposal!
#10:
For now, I've worked around this by explicitly switching to the anonymous user during rendering and then ignoring any user-related cache contexts.
I like that. Maximal cacheability for now. Let's defer per-user previews to a follow-up, i.e. this bit: β that is indeed the truly correct solution. (It's basically "placeholdering support" β but instead of markup in a render array tree, it's metadata blobs that end up encoded as JSON).
Reviewingβ¦
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Looking great!
I think this is very close. Basically all of my feedback is trivial. So I pre-emptively approved this MR β I trust @longwave to address my feedback :)
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π
I think just capturing what's written on the MR about the consequences of
experience_builder_config_schema_info_alter()
in its docblock is sufficient to make this RTBC and commit it? ππ - π¬π§United Kingdom longwave UK
Added some additional comments and a todo linking to the repurposed π Define `props` in the context of Block components Active
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
#10 WRT per-user caching, I +1'd it in #12, but having finished #3485917, I now think that:
- #3485917 should land first, see π `ComponentTreeMeetRequirements` constraint validator must be updated now that Blocks-as-Components are supported Active for why
- We actually can do better here: allow
user.permissions
,user.roles:authenticated
etc. (which all work fine with Dynamic Page Cache, and result in a reasonable number of variations), and disallow onlyuser
(aka "per user"). See https://git.drupalcode.org/project/experience_builder/-/merge_requests/3... for my detailed proposal.
- π¬π§United Kingdom longwave UK
I've merged in π `ComponentTreeMeetRequirements` constraint validator must be updated now that Blocks-as-Components are supported Active and closed π Decide how to handle block previews per user Postponed as that's implemented here now.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Proposed one important generalization that guarantees the responsiveness of this API response that is so very critical for the XB UX.
See https://git.drupalcode.org/project/experience_builder/-/merge_requests/3....
Approved MR, pending your agreement, @longwave β feel free to merge if you agree with what I pushed π
- π¬π§United Kingdom longwave UK
Simplified the use of
match()
and shortened some of the rest of the code, this is ready to go now for me. - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
RTBC++, but it's failing tests on 10 (passing on 11).
Investigating, and hopefully merging soonβ¦ π
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Baby duty called β @longwave beat me to it! π
-
wim leers β
committed fdbbd78d on 0.x authored by
longwave β
Issue #3484671 by longwave, wim leers: Add support for block derivatives
-
wim leers β
committed fdbbd78d on 0.x authored by
longwave β