![Announcement icon Announcement icon](https://www.drupal.org/files/styles/grid-2-2x-square/public/announcements/drupal-evergreen-logo-280X280px%20%281%29_0.jpg?itok=PHpn6rCb)
Overview
ComponentSourceInterface::getClientSideInfo()
returns an array shape with various bits of metadata required for the component listing in the React client:
- Component ID and label
- Default HTML markup
- CSS and JS required to render the component
Is an array OK here or should we use a proper value object? 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?
Proposed resolution
ApiComponentsController
is essentially normalizing a list ofComponent
config entities- … and hence the way to bring consistency to all this is to refactor away
ApiComponentsController
, by updatingComponent
to implementXbHttpApiEligibleConfigEntityInterface
just likePattern
does - … plus probably moving
ApiConfigControllers::normalizePattern()
to a newPattern::normalize()
(on the config entity class itself!) - … and similarly add a new
Component::normalize()
to contain most ofApiComponentsController
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
.
User interface changes
None.
Issue fork experience_builder-3484678
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
Comment #2
longwave@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.
Comment #4
longwavegetClientSideInfo()
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.
Comment #5
wim leersThanks to #3482529: Refactor XB internals to not assume /xb-components HTTP API returns only SDC-powered XB Components, 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.)
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
.Comment #6
wim leersComment #7
longwaveWell, 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...
Comment #8
wim leersOf course! 😅
Agreed that would be nicer! 👍
Comment #9
longwaveLocally 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.
Comment #10
wim leersChallenge accepted 🤓
Comment #11
wim leersI lost track of this. This is still relevant and worth doing.
Comment #12
wim leers#3491021: Leverage HTML comment annotations, remove wrapping div elements is touching adjacent areas and resurfaced the need for this clean-up.
Merging in
origin/0.x
, with ~1.5 month worth of changes 🫣Comment #13
wim leersIn #3486203: Integrate saving sections with the backend: (de)normalize Pattern config entities using the client-side data model, 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 :)
Comment #14
wim leersAnd just 2 days ago, #3491021: Leverage HTML comment annotations, remove wrapping div elements also added a
@todo
pointing here!Comment #15
wim leersAlright, 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?
PropSourceEndPointTest
, which is failing in the way Dave described:\Drupal\experience_builder\Controller\ApiConfigControllers::normalizePattern()
, which pretty much duplicates what\Drupal\experience_builder\Controller\ApiComponentsController::getCacheableClientSideInfo()
does.Comment #16
wim leersThe 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 byEarlyRenderingControllerWrapperSubscriber
.But why? 🤔 Debugging…
Comment #17
wim leersNote 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
with the condition
$data['#cache_properties'] == ['title', 'changed']
, and you'll see this before the::render()
call:and afterwards,
max-age
has changed to1
. 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:… 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.
Comment #18
wim leersWhy this was not happening previously: the
call was wrapped in
::executeInRenderContext()
, and hence theViewsBlock::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...
Comment #20
wim leersThe 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 withComponent
s, but withPattern
s. So while it has to generate the same key-value pairs (default_markup
,css
,js_header
andjs_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 ofComponent
config entitiesApiComponentsController
, by updatingComponent
to implementXbHttpApiEligibleConfigEntityInterface
just likePattern
doesApiConfigControllers::normalizePattern()
to a newPattern::normalize()
(on the config entity class itself!)Component::normalize()
to contain most ofApiComponentsController
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 generatingdefault_markup
,css
,js_header
andjs_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
I'm totally fine with just landing the original MR right now and deferring the rest to a follow-up 😊 But @larowlan seemed to feel strong about one particular detail (despite the net LoC reduction mentioned above), so I'm also fine with doing this here, especially because I've proven it's totally within reach.
Goals from the issue summary
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.I think it's okay as an interim step, but if you don't, then see the "alt" MR from above.
Comment #22
longwaveLGTM, I like the additional changes in MR!547 - RTBC once the nits from the review are done.
Comment #23
wim leersYAY, many thanks, @longwave!
This:
default_markup
/css
/… that landed <24 hours ago in #3486203: Integrate saving sections with the backend: (de)normalize Pattern config entities using the client-side data model\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 #3484671: Add support for block derivatives, to the very UX that only landed <24 hours ago\Drupal\experience_builder\Controller\ApiConfigControllers
::getClientSideInfo()
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
intoXbConfigEntityHttpApiTest::testComponent()
, but there's so much cruft in there anyway that's going to have to change for #3455629: [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model fairly soon, that I think "just" this MR is progress enough.If your git archeology lands you on this issue, please check the https://git.drupalcode.org/project/experience_builder/-/merge_requests/547, in which I tried to pre-emptively answer all reviewer Qs and tricky bits.
comments I left on the MR atComment #25
wim leersComment #26
lauriiiI'm getting following error after this issue:
It doesn't seem that
\Drupal\experience_builder\Entity\Component::normalizeForClientSide
is providing the required values 🤔Comment #27
wim leers#26: Fixed in #3500109: HEAD broken with asserts enabled — went unnoticed due to core regression. See also #3500110: CI: PHP asserts not running in CI/during tests due to core regression. Root cause is a critical core regression: #3398033-31: Fix deprecated assert.active directive.
Comment #28
lauriiiThank you @wim leers! 🙏
Comment #29
wim leersFor the "should" in #23: #3499536-7: Move PropSourceEndpointTest to XbConfigEntityHttpApiTest::testComponent().