Overview

#3475584: Add support for Blocks as Components explicitly did not allow block derivatives. These will be required, as we want to be able to place menu blocks and Views blocks inside the page, and both of these use derivatives.

Proposed resolution

Additionally create individual component config entities for all derivatives of all blocks.

Remaining tasks

  • Open core issues to make menu and views blocks FullyValidatable.
  • Decide what to do about blocks that vary, especially per-user.

User interface changes

More blocks will be available in the UI.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

longwave created an issue. See original summary.

longwave’s picture

Assigned: Unassigned » longwave
Status: Active » Needs work

Does not look like this is too tricky, except for the FullyValidatable requirement, which is not defined for almost any block in 10.x.

longwave’s picture

Status: Needs work » Needs review

Menus and views blocks are available in the UI, menus have some basic test coverage.

longwave’s picture

Assigned: longwave » Unassigned
longwave’s picture

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.

wim leers’s picture

Assigned: Unassigned » longwave
Status: Needs review » Needs work
Issue tags: +Configuration schema, +validation
Related issues: +#3379725: Make Block config entities fully validatable

#6 I bet that it's a TRUE being cast to 1.

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, #3379725: Make Block config entities fully validatable 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 are FullyValidatable; 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.

EDIT: and XB requiring configurable block plugins to be fully validatable is an excellent motivator for both contrib and the rest of core to work on making more config fully validatable — see https://wimleers.com/talk/config-validation-drupalcon-portland-2024 🤓

longwave’s picture

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 in StylePluginBase::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.

longwave’s picture

Finally tracked it down. @Wim unfortunately you lost that bet!

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 timestamp only displays "X seconds ago", and in turn the max-age of the field is set to 1 second in DateFormatter::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. We probably want to consider setting it a bit higher even, if the previews are 5 minutes or maybe even 1 hour out of date it doesn't matter here. We have cache tags anyway if something changes!

longwave’s picture

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.

longwave’s picture

Assigned: longwave » Unassigned
Issue summary: View changes
Status: Needs work » Needs review
wim leers’s picture

Assigned: Unassigned » wim leers

#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: Maybe a better answer is to cache each preview separately, then we can vary per-user blocks without having to re-render static components. — 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…

wim leers’s picture

Assigned: wim leers » longwave
Status: Needs review » Needs work
Issue tags: +Needs followup

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

longwave’s picture

Status: Needs work » Needs review
Issue tags: -Needs followup
longwave’s picture

Assigned: longwave » Unassigned
wim leers’s picture

Assigned: Unassigned » longwave
Status: Needs review » Needs work

🏓

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? 😊😄

longwave’s picture

Assigned: longwave » Unassigned
Status: Needs work » Needs review

Added some additional comments and a todo linking to the repurposed #3484666: Show configuration forms for Block components

wim leers’s picture

#10 WRT per-user caching, I +1'd it in #12, but having finished #3485917, I now think that:

  1. #3485917 should land first, see #3485917: `ComponentTreeMeetRequirements` constraint validator must be updated now that Blocks-as-Components are supported + make PageTemplate render main content! for why
  2. 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 only user (aka "per user"). See https://git.drupalcode.org/project/experience_builder/-/merge_requests/3... for my detailed proposal.
longwave’s picture

longwave’s picture

Issue summary: View changes
wim leers’s picture

Assigned: wim leers » longwave
Status: Needs review » Reviewed & tested by the community

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 😊

longwave’s picture

Assigned: longwave » Unassigned

Simplified the use of match() and shortened some of the rest of the code, this is ready to go now for me.

wim leers’s picture

Assigned: Unassigned » wim leers

RTBC++, but it's failing tests on 10 (passing on 11).

Investigating, and hopefully merging soon… 😊

wim leers’s picture

Assigned: wim leers » Unassigned

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’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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