The 2026 Drupal Business Survey is open. Your response will help shape the future of the Drupal business ecosystem.Overview
Discovered in #3500083: Managing components sourced as code components which was doing a PATCH request to rename a code component. The request only sent the name and id. Because \Drupal\experience_builder\Controller\ApiConfigControllers::patch called \Drupal\experience_builder\Entity\JavaScriptComponent::denormalizeFromClientSide with the data from the client and that method has lines like 'props' => $data['props'] ?? [], which will wipe out the props if it was not sent from the client
Proposed resolution
Don't require the client to send all the keys. Maybe add an optional $entity argument to denormalizeFromClientSide that would set on PATCH requests
User interface changes
Issue fork experience_builder-3508140
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:
changes
Comments
Comment #3
wim leers@hoorooomoo was the one to discover it, crediting them :)
Comment #4
balintbrewsIncreasing priority, because we need to solve this in order to fix the rather critical #3513147: Using actions from the contextual menu from the sidebar list overrides code component with its latest non-autosaved version.
Comment #6
longwavedenormalizeFromClientSide()doesn't have to be static, instead we can convert it to an instance method that updates the instance directly.Comment #8
longwaveMR as POC of #6, still needs additional test coverage.
Comment #9
longwaveComment #10
longwaveComment #11
tedbowReviewing
Comment #12
tedbowReviewing
Comment #13
longwaveComment #14
wim leersAdditional feedback. 🏓
Comment #15
wim leersAlso, what a major oversight of mine! 🫣😬
Comment #16
longwaveI think the CodeComponents POST vs PATCH solution is relatively neat but wondering what other opinions are.
It's a shame really that we have to do some of this mapping in the first place, and that we use
idandmachineNameinterchangeably, and similarlynameandlabel. Should we try to clean this up before we have a stable API?Comment #17
effulgentsia commentedComment #18
wim leersOne more set of OpenAPI adjustments needed, then this will be RTBC.
The idea/intent was that the client-side and server-side can evolve independently. Same spirit as
docs/adr/0005-Keep-the-front-end-simple.md, but applied to XB's config, instead of XB's component tree data model on the server (must juggle a lot of complexity) vs client (very little complexity).I do agree there's relatively little value in it, but also not that much harm/complexity.
No strong feelings, but perhaps the argument should be that the client MUST be kept in sync to avoid the renaming on the server side, because that does not cause significant complexity on the client?
Comment #19
longwaveNo strong feelings either, just the usual "naming things is hard" argument - although I hope this decision doesn't come to bite us in the future.
Added NewAssetLibrary and NewPattern schemas and removed the separate hardcoded schemas for the POST cases.
Comment #20
wim leers💯
What decision in particular do you fear potential consequences for?
Is it https://git.drupalcode.org/project/experience_builder/-/merge_requests/8...?
AFAICT you solved both mine and @tedbow's concerns? 😄
RTBC'ing and assigning to @tedbow because I sense I'm missing something… 🫣
Comment #22
tedbowThanks all!
Comment #23
wim leersThis unblocked #3513147: Using actions from the contextual menu from the sidebar list overrides code component with its latest non-autosaved version!
Comment #24
tedbowComment #25
nagwani commented