Add support for block derivatives

Created on 30 October 2024, 16 days ago

Overview

πŸ“Œ [PP-1] Add support for Blocks as Components Active 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.

User interface changes

More blocks will be available in the UI.

πŸ“Œ Task
Status

Active

Version

0.0

Component

Config management

Created by

πŸ‡¬πŸ‡§United Kingdom longwave UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue 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
  • πŸ‡¬πŸ‡§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 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, πŸ“Œ 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 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.

  • πŸ‡¬πŸ‡§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 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.

  • πŸ‡¬πŸ‡§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 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.

  • πŸ‡¬πŸ‡§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.

  • πŸ‡¬πŸ‡§United Kingdom longwave UK
  • πŸ‡§πŸ‡ͺ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 :)

  • πŸ‡¬πŸ‡§United Kingdom longwave UK
  • πŸ‡¬πŸ‡§United Kingdom longwave UK
  • πŸ‡§πŸ‡ͺ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:

    1. #3485917 should land first, see πŸ“Œ `ComponentTreeMeetRequirements` constraint validator must be updated now that Blocks-as-Components are supported Active 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.
  • πŸ‡¬πŸ‡§United Kingdom longwave UK
  • πŸ‡¬πŸ‡§United Kingdom longwave UK
  • πŸ‡§πŸ‡ͺ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! πŸ˜„

  • Pipeline finished with Skipped
    1 day ago
    #339122
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
Production build 0.71.5 2024