Improve or remove ComponentSourceInterface::getClientSideInfo()

@larowlan also noted that this method handles cache tags specially, why only cache tags and not contexts or max-age? If possible we should remove this flag entirely here.

getClientSideInfo() now returns a build array where the HTML, CSS and JS can be extracted from. I also removed the cache tags flag as I haven't seen any problems with it locally.

We still need somewhere to get/put the props, slot data, and dynamic field candidates, but maybe those should become individual methods on the source plugin.

Is an array OK here or should we use a proper value object?

Thanks to 📌 Refactor XB internals to not assume /xb-components returns only SDC-powered XB Components Active , this is now a well-defined array shape.

Refactoring to a value object is fine, but what is the benefit? (Other than the well-defined array shape currently only being enforced using PHPStan, but that could easily be changed.)

Or can we even refactor this away entirely and just ask the component source plugin to render the component to a build array, and handle everything in the caller?

It's the client side that needs all the separate pieces of information; a "build array" (🤔 aka render array, right?) hence does not get us any further AFAIK.

See ApiComponentsController.

Well, if the source plugin returns just a render array, then ApiComponentsController can be responsible for rendering it and extracting the CSS/JS, no? See https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

Of course! 😅

Agreed that would be nicer! 👍

Locally the max-age is 1 instead of 3600 now. The original max-age is now bubbling up to the actual response even though we try to override it in the controller, but I don't see where the logic has changed for this to happen. Assigning to Wim in the hope that he has ideas or better renderer debugging skills than I do.

Challenge accepted 🤓

I lost track of this. This is still relevant and worth doing.

✨ Leverage HTML comment annotations, remove wrapping div elements Active is touching adjacent areas and resurfaced the need for this clean-up.

Merging in origin/0.x, with ~1.5 month worth of changes 🫣

In ✨ [PP-1] Integrate saving sections with the backend Postponed , I asked @f.mazeikis to add a @todo pointing here, because here is an additional occurrence of this pattern.

That makes this issue more important.

#3486203 landed last night, so pushing this forward now :)

And just 2 days ago, ✨ Leverage HTML comment annotations, remove wrapping div elements Active also added a @todo pointing here!

Alright, now we're mostly back to the state @longwave left this at 2 months ago at #9, where he assigned it to me.

Where are we?

@longwave's MR got us to the point where ComponentSourceInterface::getClientSideInfo() implementations are pretty much pointless, because \Drupal\experience_builder\Controller\ApiComponentsController::getCacheableClientSideInfo() now does centrally what each implementation duplicated 👍

What's next?

1. The sole failing test is PropSourceEndPointTest, which is failing in the way Dave described:
   

   There was 1 failure:
   1) PropSourceEndpointTest::test
   Behat\Mink\Exception\ExpectationException: Current response header "X-Drupal-Cache-Max-Age" is "1", but "3600" expected.
   

2. ✨ [PP-1] Integrate saving sections with the backend Postponed introduced \Drupal\experience_builder\Controller\ApiConfigControllers::normalizePattern(), which pretty much duplicates what \Drupal\experience_builder\Controller\ApiComponentsController::getCacheableClientSideInfo() does.

The root cause for #9/#15.1 can be found by putting a conditional breakpoint on \Drupal\Core\Cache\Cache::mergeMaxAges(), with this condition: !empty($max_ages) && min($max_ages) === 1.

Doing so reveals that this occurs when the views_block:content_recent-block_1 block is being rendered; it is being caught by EarlyRenderingControllerWrapperSubscriber.

But why? 🤔 Debugging…

Note that #9 is only (easily) reproducible in the test. This is an important hint!

Turns out that Views' rendering pipeline leaks cacheability when simply calling ViewsBlock::build():

(I bet there's a core issue for this somewhere …

buildRenderable

Put a conditional breakpoint on

          if ($renderer->hasRenderContext()) {
            $renderer->render($data);
          }

with the condition $data['#cache_properties'] == ['title', 'changed'], and you'll see this before the ::render() call:

array(4) {
  ["keys"]=>
  array(5) {
    [0]=>
    string(5) "views"
    [1]=>
    string(6) "fields"
    [2]=>
    string(14) "content_recent"
    [3]=>
    string(7) "block_1"
    [4]=>
    string(64) "86fc8fe8b57f924155e998c4788f7e8badf0989a44a5ae755a20ee7d6d15ec99"
  }
  ["tags"]=>
  array(3) {
    [0]=>
    string(6) "node:1"
    [1]=>
    string(6) "user:0"
    [2]=>
    string(32) "config:views.view.content_recent"
  }
  ["max-age"]=>
  int(-1)
  ["contexts"]=>
  array(0) {
  }
}

and afterwards, max-age has changed to 1. We're getting close!

Then I noticed that in the rendered result it said 55 seconds ago. That is obviously time-sensitive. That's the "timestamp ago" formatter.

So I put a breakpoint in \Drupal\Core\Field\Plugin\Field\FieldFormatter\TimestampAgoFormatter::formatTimestamp(), right where it has this line:

CacheableMetadata::createFromObject($result)->applyTo($build);

… and sure enough: that's where it's coming from! And now it makes sense why it doesn't (tend) to happen in a dev environment, and only in tests: because in a test, all content is freshly created.

🚨 But why was this not happening previously? Investigating now.

Why this was not happening previously: the

$component->getComponentSource()->getClientSideInfo($component);

call was wrapped in ::executeInRenderContext(), and hence the ViewsBlock::build() was bubbling cacheability inside the render context that ::getCacheableClientSideInfo() was constructing.

The fix then is simply to continue to do that (but add a comment to avoid another person having to spend >1 hour debugging this): https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

The current MR is green, and has 7 files, +119, −79 as its diffstat. But 50 of the additions are just for updating test expectations because now there's less magic! So, really, this is a net reduction in LoC!

Next

Next up: #15.2!

But \Drupal\experience_builder\Controller\ApiConfigControllers::normalizePattern() is not dealing with Components, but with Patterns. So while it has to generate the same key-value pairs (default_markup, css, js_header and js_footer), none of the code is comparable.

So actually, there is nothing to do in that code in this issue.

However, it made me realize that:

• ApiComponentsController is essentially normalizing a list of Component config entities
• … and hence the way to bring consistency to all this is to refactor away ApiComponentsController, by updating Component to implement XbHttpApiEligibleConfigEntityInterface just like Pattern does
• … plus probably moving ApiConfigControllers::normalizePattern() to a new Pattern::normalize() (on the config entity class itself!)
• … and similarly add a new Component::normalize() to contain most of ApiComponentsController

That would then also open the door to adding a new ClientSideRepresentation value object, which should accept a render array, which would take care of automatically generating default_markup, css, js_header and js_footer.

So, I proposed an "alt" MR, that builds on top of the MR @longwave started: https://git.drupalcode.org/project/experience_builder/-/merge_requests/547

Goals from the issue summary

Or can we even refactor this away entirely and just ask the component source plugin to render the component to a build array, and handle everything in the caller?

We can't yet, because SingleDirectoryComponent::getClientSideInfo() is returning too many other key-value pairs. Getting all of those addressed here would make the scope too big

Is an array OK here or should we use a proper value object?

I think it's okay as an interim step, but if you don't, then see the "alt" MR from above.

longwave → changed the visibility of the branch 3484678-improve-or-remove to hidden.

LGTM, I like the additional changes in MR!547 - RTBC once the nits from the review are done.

YAY, many thanks, @longwave!

This:

• gets rid of the duplication for default_markup/css/… that landed <24 hours ago in ✨ [PP-1] Integrate saving sections with the backend Postponed
• gets rid of some of the oldest code we had and generalized it over to \Drupal\experience_builder\Controller\ApiConfigControllers::list(), and in doing so, it brings the same performance optimizations to the list of Sections in the UI that the list of Components in the UI has enjoyed ever since 📌 Add support for block derivatives Active , to the very UX that only landed <24 hours ago
• reduces the scope for 📌 Make all XB HTTP API routes consistently prefixed Active
• achieves the original goal: simplifying ::getClientSideInfo()
• with 21 files, +369, −338 as the diffstat, although ~50 LoC are just updated unit test expectations for more explicit/less magic in the generated render arrays 👍

We should still refactor PropSourceEndpointTest into XbConfigEntityHttpApiTest::testComponent(), but there's so much cruft in there anyway that's going to have to change for 🌱 [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model Active fairly soon, that I think "just" this MR is progress enough.

If your git archeology lands you on this issue, please check the comments I left on the MR at https://git.drupalcode.org/project/experience_builder/-/merge_requests/547, in which I tried to pre-emptively answer all reviewer Qs and tricky bits.

I'm getting following error after this issue:

AssertionError: assert(!array_key_exists('default_markup', $this->values)) in assert() (line 29 of /var/www/html/drupal/modules/custom/experience_builder/src/ClientSideRepresentation.php).

It doesn't seem that \Drupal\experience_builder\Entity\Component::normalizeForClientSide is providing the required values 🤔

#26: Fixed in 🐛 HEAD broken with asserts enabled Active . See also 📌 PHP asserts not running in CI/during tests Active . Root cause is a critical core regression: #3398033-31: Fix deprecated assert.active directive → .

Thank you @wim leers! 🙏

For the "should" in #23: #3499536-7: Move PropSourceEndpointTest to XbConfigEntityHttpApiTest::testComponent() → .