🇧🇪Belgium @wim leers

Per the issue summary, this is clearly in the critical path for many things. ⇒ Bumped to priority + tagged .

Great to see this approaching the finish line! 😄🏁

Great! 😄🥳

It shouldn't matter where the image is coming from: an image field or a media field, or even an Instagram or Flickr photo — it's up to the individual field to then map these restrictions to corresponding validation constraints.

(Prior art of exactly that: search the XB codebase for the string FileExtension, and you'll find a similar example for ensuring an image URI is a HTTP(S) URI pointing to an image.)

Three things remain then:

1. @mherchel confirming he also likes #5 🤞
2. verifying that the reuse-but-override-subset of schema stuff is actually how JSON schema works 😅
3. working PoC

Thanks, @parthbcharya! 😄🙏

Both the CSS and JS are causing CI jobs to fail because they don't meet the coding standards. Should be a trivial fix compared to everything else you've done! 😊

Here the hardcoded date format ('d MMM') is working fine but it's better that the date formatting should align with Drupal’s standards to maintain consistency across the platform.

There is no one standard.

I bet you're referring to core/modules/system/config/install/core.date_format.short.yml and other core.date_format.*.yml files in that directory.

The challenges in switching to one of those would be:

1. Ensuring the patterns that PHP support are also supported on the client-side.
2. What if the chosen DateFormat config entity is deleted? ← mitigated by using a locked DateFormat, which \Drupal\system\DateFormatAccessControlHandler disallows being deleted. Not a perfect guarantee, but probably good enough. ✅
3. Bikeshedding which DateFormat config entity the XB UI would use 😜

For point 2: safer/better still would be to make XB depend on that DateFormat config entity, but that'd require a XbUiConfig config entity or something like that, which would declare various config dependencies on behalf of XB. But … perhaps that'd actually be a good thing? There are likely more needs like this! That'd even allow a single central "XB configuration" UI (in an optional submodule? or contrib module?) for these kinds of settings.

Asking @lauriii for feedback for that last paragraph :)

👏

✨ Allow specifying `meta` data on JSON:API objects Needs work is in!

@larowlan is right, we can simplify more here thanks to 📌 Upgrade twig/twig to 3.15.0 Active !

FYI: Anybody could've merged this!

But, taking a practical look, and without investigating whether this is how ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed currently works, but assuming we could make it work like this (unsure about it), I'd propose something like this:

diff --git a/schema.json b/schema.json
index 12d5c628..30a133d6 100644
--- a/schema.json
+++ b/schema.json
@@ -200,11 +200,13 @@
         },
         "width": {
           "title": "Image width",
-          "type": "integer"
+          "type": "integer",
+          "minimum": "0"
         },
         "height": {
           "title": "Image height",
-          "type": "integer"
+          "type": "integer",
+          "minimum": "0"
         }
       }
     },
diff --git a/tests/modules/sdc_test_all_props/components/all-props/all-props.component.yml b/tests/modules/sdc_test_all_props/components/all-props/all-props.component.yml
index 43c5c9a1..c0c9c674 100644
--- a/tests/modules/sdc_test_all_props/components/all-props/all-props.component.yml
+++ b/tests/modules/sdc_test_all_props/components/all-props/all-props.component.yml
@@ -198,11 +198,25 @@ props:
       $ref: json-schema-definitions://experience_builder.module/image
       # @todo Remove in https://www.drupal.org/project/drupal/issues/3352063 — should not be necessary, but removing this causes \Drupal\Core\Theme\Component\ComponentMetadata::parseSchemaInfo() to generate `type = ['', 'object'] 🐛
       type: object
+      width:
+        minimum: 500
       examples:
         - src: https://placehold.co/600x400.png
           alt: 'Boring placeholder'
           width: 600
           height: 400
+    test_object_drupal_image_big:
+      title: 'Object, $ref=json-schema-definitions://experience_builder.module/image, overridden minimum width'
+      $ref: json-schema-definitions://experience_builder.module/image
+      width:
+        minimum: 1000
+      # @todo Remove in https://www.drupal.org/project/drupal/issues/3352063 — should not be necessary, but removing this causes \Drupal\Core\Theme\Component\ComponentMetadata::parseSchemaInfo() to generate `type = ['', 'object'] 🐛
+      type: object
+      examples:
+        - src: https://placehold.co/1000x400.png
+          alt: 'BIG HERO IMAGE'
+          width: 1000
+          height: 400
 
     # The "date-range" object format for this module.
     test_object_drupal_date_range:

WDYT? 😊

✨ Allow developers to customize how components behave in XB Active is news to me. Responded there.

Image is already a capability that's specific to Experience Builder. Are we really blocked on core to make this change?

It isn't, not really. Literally all it relies upon is ✨ [PP-1] Allow schema references in Single Directory Component prop schemas Postponed .

I think this means we'll need to modify the ADR introduced in 📌 Document the current component discovery + SDC criteria, and describe in an ADR Active as well as the product requirements.

But…

1. Components built for Drupal Core should work with Experience Builder without additional changes.

👆 this is pretty much the original product requirement you stated.

Which implies that if this must be upheld, that is the inevitable consequence?

Indeed, route-level access checking only checks entity-level access.

But … we could change that in principle, if that simplifies things: we could build a field-level equivalent of \Drupal\Core\Entity\EntityAccessCheck, which would then need to inspect all fields received in the request body.

That'd move the moment of field-level access checking to before controller execution instead of during.

Would that simplify things?

P.S.: I recall having to deal with this while working on Drupal core's REST + JSON:API modules. I remember investigating field-level access checking. I do not recall whether there is a blocker to checking field access earlier (they both checking it during controller execution). I suspect it's mostly related to the need to deserialize the received data first. But if XB's request bodies follow a firm structure where the list of modified fields is easily accessible, I think it would be possible to do field-level access checking earlier.

The issue summary (and title) still (see #14) don't convey the scope. A screenshot/screencast might help, but AFAICT this is not modify the UI, only under-the-hood things?

Shouldn't this have Cypress E2E tests? If not, or while awaiting those, how should I manually test this?

Detailed review + manual test screencast in https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

Approved.

And added

-[DX: CI] @wimleers
+[DX: CI] @wimleers @bnjmnm @larowlan @longwave

… which is long overdue.

wim leers → made their first commit to this issue’s fork.

#10++ — nice! 😄

Many questions on the MR, but this is definitely going in the right direction! 👍

Assigning to @longwave because he's actively working on it, and Lee is hopefully asleep 😊

@longwave in #7: I agree it can be useful! But definitely not always.

The majority of the comments that that rule forced us to add were not meaningful; they just restated what the name already said. In core too, more often than not, these comments are actually stale and hence (anywhere between "slightly" and "very") wrong. In XB, I think the combined use of:

• more typed objects
• PHPStan level 8, which then includes array shape definitions

massively reduces the need that once existed. I think that rule made sense in an era before those two things, but not now. The collateral damage is too great.

Confirmed: latest nightly run fails in the same way: https://git.drupalcode.org/project/experience_builder/-/jobs/3756953

Looks like Figma is down, because it never gets beyond this point:

Inspector says:

127.0.0.1:44950/figma/font-files?freetype_minimum_api_version=20&isolate=true&:1        Failed to load resource: net::ERR_CONNECTION_REFUSED

😅

📌 Define `props` in the context of Block components Active landed!

Also, https://git.drupalcode.org/project/experience_builder/-/merge_requests/3...

This unblocked 📌 Implement saving block settings forms Active !

#2++, tried to clarify slightly :D

Bumping priority because this will continue to cause confusion otherwise.

@longwave, any chance you could get an initial MR up in the next few days, so that we can eliminate any further wasted minutes/hours on this confusion? 😇🙏

📌 Add support for global regions Active is in!

🥳

See y'all in:

1. 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active
2. 📌 Implement auto-save of the page template config entity Active
3. 📌 Allow source components to control how hydrated values are mapped into the clientside model Active
4. 📌 [PP-1] Add contextual menu frontend support for moving components into regions Postponed
5. And last but not least: @longwave is going to file a follow-up for https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... — we hopped on a Zoom to clarify that :)

(@longwave please link that issue and mark this as when you've done that!)

Post-humously bumped to considering how many issues this unblocked! 😅

📌 Add support for global regions Active is in!

📌 Add support for global regions Active is in!

📌 Add support for global regions Active is in!

I'm sure this looks nitpicky … but I'm just trying to use consistent terms to make it easier to spot connections.

@longwave in #33: to have it available in the annotation-based plugin definition' but also use that information from the plugin definition in the code. This is nothing new; I'm just matching the existing pattern that you and @f.mazeikis introduced (see BlockComponent::SOURCE_PLUGIN_ID) in 📌 Add support for Blocks as Components Active ? 😅

In order to not violate @lauriii's product requirements (nutshell version: "no XB-specific metadata in SDCs"), this would have to be expressed in SDC's metadata. Please move this to Drupal core's single-directory components issue queue component and link it from 📌 [SPIKE] Comprehensive plan for integrating with SDC Active 🙏😊

#3: How could it? Recipes can only install config or perform config actions. A container parameter is outside the realm of config.

Or … is my head so far in baby/diaper/poop land that the poop has affected my thinking? 🤣💩

1. 📌 Add setting to enable XB for page templates Active is in.
2. As is 📌 Create a value object for the return of ComponentTreeHydrated::getValue Active , which @larowlan alluded to in his review.
3. 4 E2E tests are still failing.
4. I pushed one proposed commit (feel free to revert altogether) to outline how I think we should address @larowlan's review at https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

=

wim leers → changed the visibility of the branch 3489899-add-global-regions to hidden.

I can locally reproduce the cypress E2E test failures:

      cons:error ✘  TypeError: Cannot read properties of undefined (reading 'values')

I'm sure that takes somebody actively working on the UI an order of magnitude less time to fix than it does for me 😅

I already approved this MR 4 days ago in #20 — that's still true: I'd love to see this land sooner rather than later! 😄

💯

So the endpoint actually just receives a plain, arbitrary JSON object, then? If so, this looks fine to me.

Really?! 😳🤔

I don't think that's the case.

I think @larowlan was only trying to demonstrate that /openapi.yml was not updated in 📌 Change ApiContentUpdateForDemoController to save from auto-save instead of request data Active , yet still it allowed a seemingly invalid request to be considered acceptable.

@larowlan, is this a duplicate of/a subset of 📌 See why OpenAPI mistake for API error didn't cause test failures Active ?

MR approved!

ComponentTreeHydratedTest needs its expectations to be updated for this to pass. Once green, go ahead and merge 😊

$ npm run drupaldev
> vite-template-redux@0.0.0 drupaldev
> VITE_DRUPAL=true vite
  VITE v5.4.11  ready in 289 ms
  ➜  Local:   http://localhost:5173/
  ➜  Network: use --host to expose

… oops, npm run drupaldev is long-running 😅

I'll let @balintbrews adjust that! 🤓

@balintbrews in #11: wow! 😳 Quite the rabbit hole! And 👏🥳 for #13!!!

Idea: if we run the UI test suite with both dev and prod builds of the UI, we'd have been able to catch this automatically! Issue + MR: 📌 CI: new `cypress E2E debug` job Active .

wim leers → created an issue.

Sorry to have wasted some of your time, @utkarsh_kumar_singh 🙈

This was a lesson for me as well, I learned something new about Drupal core here! 😅

Your effort does not go unnoticed nor unappreciated though: you will be credited for this issue! 😊

Retitling & rescoping.

Ugh … I was wrong 🙈😬

Due to

  public function __construct(…) {
    parent::__construct('Entity', $namespaces, $module_handler, 'Drupal\Core\Entity\EntityInterface');
    …
    $this->discovery = new AnnotatedClassDiscovery('Entity', $namespaces, 'Drupal\Core\Entity\Annotation\EntityType');
    …
  }

in \Drupal\Core\Entity\EntityTypeManager::__construct(), it is impossible to put (content or config or otherwise) entity definitions in any other place than src/Entity! 🤯

This is highly misleading in Drupal core, because, these critical files do live in a …/Config/Entity/… subdirectory:

• Drupal\Core\Config\Entity\ConfigEntityInterface
• \Drupal\Core\Config\Entity\ConfigEntityBase
• …

😬

I just checked /node/add/article in Drupal core and saw:

So let's let @bnjmnm have the final say — he's an Accessibility maintainer in Drupal core.

7 years and 2 days later, this core bug is still biting us: 🐛 Blocks are missing their block.html.twig wrapper Active .

// @todo Move this to BlockBase in https://www.drupal.org/node/2931040.

🤯

This has basically been a core bug for more than a decade!

Also, AFAICT this relates to 📌 Move form state into the global store Active too? Is that a third kind of value?

• raw: as stored in a FieldType plugin on the server side
• resolved: as resolved into a shape expected by an SDC prop
• form: as represented by a FieldWidget for a FieldType, aka 📌 Move form state into the global store Active

… or am I misinterpreting things now? 😅

Split the component model values into raw and resolved.

To make sure I don't misunderstand: you're saying that this is necessary on the client-side, i.e. in the UI's data model?

#5++

Should we also be changing FormElementLabel to emit something visually hidden for screen readers too?

— @larowlan

+1

This also conflicts with 📌 Move form state into the global store Active , which landed on Saturday. The conflict is simple enough to resolve, but IDK yet if tests will pass. Let's find out! 🤠

Be back after lunch…

Shoot, this MR was green and seemingly ready for (re-)review since Friday night! Will get on it.

I'm not opposed to that at all, but @longwave, @jessebaker and others felt that 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active was too broad in scope to be actionable. If you managed to act on it anyway, all the better! 😄

I suspect we'll need to resolve this too, the @todo points here

— @larowlan at https://git.drupalcode.org/project/experience_builder/-/merge_requests/3..., commenting on BlockComponent::hydrateComponent()

Lee is right.

BUT!

#2 is not true anymore, because 📌 Add setting to enable XB for page templates Active was forced to land a low-level piece of this: it needed \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::hydrateComponent() to handle block plugin settings and
— see https://git.drupalcode.org/project/experience_builder/-/merge_requests/4...

Still, we need this MR to be rebased and ensured that that is working as intended with all changes of this MR.

@larowlan previously opened an issue for this: 🐛 \Drupal\experience_builder\Plugin\DataType\ComponentTreeHydrated::getValue should not return a response object Active — this was effectively a newer duplicate 😊

@larowlan duplicated himself and landed it: 📌 Create a value object for the return of ComponentTreeHydrated::getValue Active 😄🚢

The bug reported by @effulgentsia is another reason this merits being critical: 📌 LayoutPreview schema should have required properties Active .

I think this is another reason for 📌 See why OpenAPI mistake for API error didn't cause test failures Active to be critical.

AFAICT a settings.php setting is impossible, because that'd then mean that everybody working on XB would have to set that 😅

Based on the requirements in the issue summary, I think the following implementation would make sense:

• container parameter
• set by a hidden module
• which is installed by a recipe

Tried it and … nope, any already-installed module appears at /admin/modules/uninstall, even if it gains a hidden: true flag. That also means that the hidden submodule choice is equally simple to undo.

So then … I think still doing what I propose above, but labeling the module or something like that would be reasonable? (Perhaps even without the scary/silly suffix.)

Thanks, @larowlan, if that's it, then that's a very important MR to land ASAP.

(It's unfortunate that @TravisCarden has been called away from XB for a while now, to work on Automatic Updates-related things. If he hadn't, this probably would've been fixed a long time ago 😭 And it'd have saved a lot of pain!)

Can you define "broken"? 🙏

Thanks!

I'd love to see @larowlan approve this too, or suggest an alternative 🤞

📌 Add setting to enable XB for page templates Active is in!

HAH! This needs @mglaman's approval due to

/src/Entity/Page*

for the change to

src/Entity/PageTemplate.php

Follow-up issue+MR to fix that: 📌 Move config entity classes+interfaces from `\Drupal\experience_builder\Entity` to `\Drupal\experience_builder\Config\Entity` Active . Too much noise to do here.

wim leers → created an issue.

Yay for baby feedings! 😁🍼

Root cause found.

Was surprisingly tricky!

It's late now, I'll finish this tomorrow most likely.

I'm getting close to figuring out the root cause of #17: the local tasks block appears once in Claro's header region, and once in its pre_content region. The first time with primary: true, secondary: false, the second time the other way around.

@larowlan's settings work does reflect those different settings per instance 👍, see:

Yet:

There must be some static caching or incorrect ID matching going on, should be a trivial fix. But first: dinner.

This should allow me to revert the AdminContext check.

@effulgentsia in #6:

when you worked on that, we didn't know yet whether we'd be using Workspaces at all.

Indeed! It was a possibility. So what I did was meant to be possible to be used independent of the Workspaces module, but also be possible to be used with it. @alexpott spotted that too: see #3493461-17: Identify roadblocks to staging config in Workspaces (e.g., via wse_config) → and #7 😊

@alexpott in #7: RE: config translations. Damn!!!! 👏👏👏 That's a very good point, and one that I missed in the PoC I did 😔 XB's config entities definitely would need translations AFAICT: plenty of labels (for component instances, patterns, etc.) that ought to be translatable.

OTOH … any config in a workspace would need to be created in the default config language, to allow it to then be translated (and config translations to be loaded by LanguageConfigFactoryOverride), right? So … does that mean that any modifications to config in XB/workspaces must happen in that language? 🤔

@amateescu in #8:

About using config overrides for workspace-specific config, I've also been thinking about it for a long time, but, as confirmed by chatting with @alexpott a few days ago, that approach can't cover the use case of adding new config.

Damn!!!! 👏👏👏 That's a very good point, and another one that I missed in the PoC I did (see link in #4). 😬

[…] wse_config is somehow seen as an immutable block of code, but it's really not..

👏🙏 Thanks for stating this, this is a very important acknowledgement!

I read this as: "please feel free to expand upon what wse_config does, and/or take only the parts that y'all think are usable for the product requirements y'all have" — does that match your intent? 😊

The design of workspace reverts in general is to allow users to revert workspace publications going back as far as possible, but only one at a time,

I read this as: "reverting a workspace publication is like reverting git commits: if you don't revert them in the reverse order of how they were applied/committed, you'll likely run into problems because there will likely be conflicts, and if not conflicts, then there may be semantical problems" — is that a fair interpretation? 🤔

@catch in #7: Great! On the one hand, I think config entities are simpler than content entities. On the other hand, content entities have a more predictable structure (a content entity has N fields, and every field has a specific validatable structure, with almost always every field value being independently validatable — the exceptions being the validation constraints that extend \Drupal\Core\Entity\Plugin\Validation\Constraint\CompositeConstraintBase), but on the other hand config entities have an arbitrarily complex structure (mappings or sequences at any level, not just the top level, so also many levels down, and even a mapping in a mapping in a mapping in a sequence etc.).
So … that makes me strongly suspect that that project is not generalizable to arbitrary config entities… 🙈

@lauriii in #8:

• RE: first paragraph: the "then merged" part still requires conflict resolution.
• RE: second paragraph: even if a new field being added to a bundle is not exposed in the UI, it would still get exposed via validation errors (for invalid field value, or potentially even missing required value) and via APIs: REST, JSON:API, GraphQL, as well as the PHP Entity/Field API. And more complex still: what if the same field gets added twice?
• RE: third paragraph: okay, thanks! 👍

@effulgentsia in #9: But the same config entity may be modified (and auto-saved) in different ways in each workspace. Or … are you saying that that is what you are proposing to simply prevent/disallow, @effulgentsia? 🤔

IOW, are you proposing: ?

@catch in #10:

we can give people different foot shooting options they can choose between.

🤣

@alexpott in #13:

the moment you edit content in a workspace you can no longer edit it in the live site. […] It's more conflict prevention than resolution I guess.

Oh, wow!

That sure is a way to avoid it! So, when trying to edit the live version of a content entity that already has a modified version in any workspace, you'd get a message informing you editing it is not possible at this time? 🤔

@lauriii, is that acceptable to you?

@alexpott in #14:

[…] the possibility that the configuration change when you publish a workspace will affect more than just the content in the workspace being being published and the same goes for a revert. I think this breaks the mental model of what a workspace encapsulates.

I'd say it's a certainty, not a possibility. The most common examples for XB will be:

1. modifying the front-end theme's PageTemplate config entity (added in 📌 Introduce an XB `PageTemplate` config entity Active ), which will affect all HTML-serving routes, not just the ones whose content entities were modified in the workspace
2. modifying a content entity type's bundle's "content type template" (does not yet exist, but is a critical product requirement — see 🌱 [later phase] [META] 7. Content type templates — aka "default layouts" — affects the tree+props data model Active ), which will affect all content entities of that type/bundle, not just the modified/added content entities in the workspace

⚠️ This is why I've been so concerned about seeing the designs for editing the PageTemplate: while 📌 Add support for global regions Active is moving forward and that is great, I have yet to see UI designs with affordances that ensure that the user modifying the PageTemplate is they're editing the page template! See my comment #3489899-9: Add support for global regions → from >2 weeks ago.

@catch in #15: you're right that this is not technically different from modifying "live config". But the point is that XB is aiming to do better and make the Drupal UX easier to understand. If we don't pay attention to that, we don't materially move the needle on this front, which has many repercussions.

@amateescu in #16:

• RE: reverting broken being trivial: yay, exactly as anticipated! 😄
• RE: → music to my ears! 😄🎶 Is it sufficient as-is? If not, what is missing?
• RE: "hiding it from the API" — /me high-fives @amateescu for raising the exact same concern! 😁🤓
• RE: caching layers pain: I have an idea. core.services.yml contains the renderer.config container parameter. It contains required_cache_contexts: ['languages:language_interface', 'theme', 'user.permissions']. If we would generalize that to all cache bins, then it'd become simple: by default, all cache bins have [] (the empty array) as the required cache contexts, and again defined in a container parameter. The Workspace module would then be able to alter the new container parameter, and add 'workspace' to it (the \Drupal\workspaces\WorkspaceCacheContext). Done!

  I think this could even be done in a backwards-compatible way. 🤔 Though it would be non-trivial to get this through the core review process and ensure strong guarantees for backwards compatibility 😬

@alexpott in #17:

not actually use config overrides but make it look like a config override is active for an config we've changed in the workspace. Then we might not have to do so many cache shenanigans in wse_config.

That's exactly what I was thinking back then! What I did in https://git.drupalcode.org/project/experience_builder/-/commit/fca2d9b54... is agnostic of where the config is stored. And in fact, I think changing that to just use the already-existing workspace cache context instead of the new-for-PoC cache context … might mean that it could quite easily be made to work with the existing wse_config? Can't quite think that all the way through at this time though, it's been too long since I worked on that.

@catch in #18: HAH, and that is exactly what I just proposed to @amateescu above! 😁

📌 Define built-in components and categorization for components Postponed is in — tangentially relates to this issue, see #4.

📌 Define built-in components and categorization for components Postponed is in.

@kristen pol Any thoughts from you on #3 until @lauriii gets a chance to respond? 😊