Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
Account created on 26 December 2006, over 18 years ago
  • Senior Principal Software Engineer β€” Drupal Acceleration Team at AcquiaΒ  …
#

Merge Requests

More

Recent comments

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Retitling for narrowed scope.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Just had a ~45 min call with @balintbrews & @effulgentsia about this, and a LOT has been cleared up by it! Much in my last comment #33 is wrong, because I didn't know about 2 key decisions/assumptions not explicitly stated in this issue yet. Witht hem #26 makes a lot more sense:

  1. we don't need to support the image_desaturate (grayscale), image_rotate etc. effects, because we just do them in CSS nowadays
  2. in fact, for the scope of this issue, we want to focus on the ~95% use case, and we do only one transformation: we scale the image down to fit the requested width

On top of this, per @effulgentsia, we can just hardcode the conversion to .webp, because that is (sufficiently) universally supported.

Then, we modify the existing image shape like this:

diff --git a/schema.json b/schema.json
index 004279ce0..ede37e558 100644
--- a/schema.json
+++ b/schema.json
@@ -205,6 +205,11 @@
         "height": {
           "title": "Image height",
           "type": "integer"
+        },
+        "candidateTemplate": {
+          "type": "string",
+          "title": "Image candidate string URL template",
+          "format": "uri-template"
         }
       }
     },

… to convey that the server should provide a string like /sites/default/files/styles/xb--{width}/public/2025-06/foo.jpg.webp?itok=1Rl59WAb (which is the image URL template for an image candidate string for srcset), where the only further information needed on the client side is the deviceSizes (which can be passed via drupalSettings).

Then the computed property that you and I discussed earlier today, @justafish (which you haven't yet pushed at this time), does start to make sense (unlike what you and I thought earlier today).

That means all other considerations in the issue summary are descoped per @effulgentsia. Including cropping and everything else listed in #34.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

AFAICT, this is almost same problem space as https://www.lullabot.com/articles/decoupled-drupal-hard-problems-image-s....

Quoting from @e0ipso's article:

In a decoupled application your back-end service and your front-end consumer are separated. […]

In this situation, we can see how our back-end doesn't know anything about the front-end(s) design(s).

we need to replace "front-end consumer" with "component (SDC, code component β€¦)" and we actually do know which components are present on a site.

The way that was solved:

After some research about how other systems tackle this, we established that we need a way for consumers to declare their presentation dependencies. In particular, we want to provide a way to express the image styles that consumer developers want for their application.

Which I believe applies here, too. But instead of the consumer/client (for XB: "component") needing to express all image styles they need, here we'd only need the subset of image styles up to a width that may be smaller than the maximum width configured for the site.

See @lauriii's in #5. Combined with Next.js' Image's deviceSizes, that'd then result in

    deviceSizes: [640, 750, 828, 1080, 1200, 1920, 2048, 3840],

being limited to

    deviceSizes: [640, 750, 828, 1080, 1200, 1920, 2048, 2500],

or, if the image was limited to 800, then it'd have been limited to:

    deviceSizes: [640, 750, 800],

If we apply @effulgentsia's #26, then I think Alex meant that:

  1. every "image-shaped prop" can benefit from automatically optimized images (i.e. anything using type: object, $ref: json-schema-definitions://experience_builder.module/image right now β€” proposal to improve that at πŸ“Œ Decouple image (URI) shape matching from specific image file types/extensions Active )
  2. for each "image-shaped prop" for each component instance, there can be at most 8 different derivatives generated on disk
  3. all component instances that reference the same file on disk (no matter whether it's a File entity for an image field, a File entity for or for an image media entity, or the default image shipped with an SDC) would result in the same 8 derivatives (not necessarily the same URLs, but presumably)
  4. each "image-shaped prop" in a component can have its own set of image operations ("image effect plugins" in Drupal) specified, because for example a "product spotlight" component might want to have front, left, right, back props, and make the front one have a much bigger/more prominent image.
  5. I think this is what he was getting at with type: string, format: uri-template near the end of #26; so that you would for a component with limitations imposed by the component developer you'd end up not with
            "src": {
              "title": "Image URL",
              "$ref": "json-schema-definitions://experience_builder.module/image-uri"
            },
            "alt": {
              "title": "Alternative text",
              "type": "string"
            },
            "width": {
              "title": "Image width",
              "type": "integer"
            },
            "height": {
              "title": "Image height",
              "type": "integer"
            }
    

    but with something like this (still figuring out how to use URI templates in JSON Schema, but there is a great package for PHP: https://uri.thephpleague.com/uri/7.0/uri-template/ β€” which I worked on with @gabesullice for AM:A at https://git.drupalcode.org/search?search=uri-template&project_id=63588&s...):

            "src": {
              "title": "Transformed image URL, transforming any input image to square aspect ratio and grayscale",
              "type": "string",
              "format": "uri-template",
              "$ref": "/xb_dynamic_image_style{/path_to_image}?effect_ratio=square&reduce_colors=grayscale&scale[width]={width}&itok={itok}",
              "x-image-max-width": 250
            },
            "alt": {
              "title": "Alternative text",
              "type": "string"
            },
            "width": {
              "title": "Image width",
              "type": "integer"
            },
            "height": {
              "title": "Image height",
              "type": "integer"
            }
    

    … which would:

    1. give the component 1 URL with most but not all placeholders replaced: {path_to_image} would be replaced (pointing to the selected image) and {itok} (as described by @effulgentsia in #26, with one itok for ALL 8 ), but NOT {width}
    2. For {width}, the client side (or the Twig equivalent) receives the set of deviceSize (or however we name our equivalent), which then enables the client to trivially generate all URLs pointing to all <=8 optimized image URLs.
    3. IOW: the server side would expand all bits only the server side can (itok + the path to the selected image)

This means:

  1. that the server can validate widths against the itok, and refuse any widths that are NEITHER in the (for now hardcoded) global/default deviceSizes NOR the max of 250 (x-image-max-width)
  2. that the server can compute which image derivatives to purge [……… more thinking needed, need to post this this …………]

P.S.: @justafish indicated that the image style infrastructure only supports File entities. But we know we'll have to make non-File entity files work as well, for example default images included in SDCs. I know for sure that this is technically possible, because I had to do this too in my https://drupal.org/project/cdn module, and there I have a route with this path:/cdn/ff/{security_token}/{mtime}/{scheme}. But we'll need be really careful: it's easy to make mistakes β€” core got it wrong too, see https://www.drupal.org/sa-core-2023-005 β†’ .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

XB is not yet being tested on 11.2, let alone with Gin.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ₯³ Thanks, @mayur-sose! Crediting you for that :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

That last commit is πŸ€©πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Looks great!

One question, plus needs a follow-up for fixing cacheability.

Already approved πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I see β€” then changing from to πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#30: correct, that is the current reality. See \Drupal\experience_builder\ComponentSource\ComponentSourceBase::generateVersionHash(). That only looks at:

    $normalized_data = [
      'settings' => $typed_source_specific_settings->toArray(),
      'slot_definitions' => $this instanceof ComponentSourceWithSlotsInterface
        ? self::normalizeSlotDefinitions($this->getSlotDefinitions())
        : [],
      'schema' => $this->getExplicitInputDefinitions(),
    ];

No Twig/markup changes can affect it.

I was just contemplating that it would be possible to either:

  1. put some image restriction-y metadata in the explicit input definitions ("JSON schema for the SDC props in *.component.yml" in SDC terminology)
  2. use infrastructure similar to how we currently generate version hashes to be able to track what image restrictions are in place, to help us purge when needed
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ› Prevent navigation in component preview Active just landed btw, so I want to emphasize how this is not that β€” see #6.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • Automatic responsive image handling - Generate appropriate srcsets for different device sizes

Won't this mean this will only be available for server-side rendered components? So it won't be available for code components (JavaScriptComponents) or really just … any component that also happens to use images? Doesn't that violate ?
β†’ ah explained later in the issue summary: , but no plan in place yet.

It should additionally provide a Twig function for easily generating URLs to processed images.

This sounds wrong to me; if it must be a Twig function, then we're essentially saying we're deviating from https://nextjs.org/docs/pages/api-reference/components/image, which contradicts the earlier statements in the issue summary. Next.js' "Image" component declares everything statically and auto-generates an appropriate URL.

(Which means I'm apparently stating almost the same as @catch >2 months ago in #10.)

@catch in #16 on April 17:

If a component defines a 2500px responsive image style which generates say five different image derivatives, then the hmac in the MR will ensure that no-one can generate a 2499px responsive image style arbitrarily. However, if a site stops using that component (swaps it out for one that's 1800px instead), how will the system know that it needs to delete all of those images (or not if the component is used elsewhere in a different layout)? This could be hundreds of thousands of derivative images on disk to worry about.

Here I have some good news:

  1. πŸ“Œ Version component prop definitions for SDC and Code components Active added a new tool to our toolbox: per-Component versions. The metadata that impacts the version hash (or put differentlY: when the hash changes) could be updated (current implementation stems from πŸ› Deterministic Component version hash should depend not only on source-specific settings, but also slots + explicit input schema Active ) could be updated to include the image restrictions imposed by the component developer.
  2. As of πŸ“Œ Calculate field and component dependencies on save and store them in an easy to retrieve format Active and its follow-ups, and especially πŸ“Œ Revisit storage of dependencies in separate table now we have separate deltas per component Active , we can determine which Component version is used in which component tree (in both config entities and in which revision of which content entity's XB field component tree).

@tonypaulbarker in #17: first you write

Not all of these should be configurable per instance by editors. We will arrive at a difficult to use UX.

which I think everybody would agree with. But then: , after which you list 7 important things content editors must be able to control.

To me this reads like a contradiction. Especially without designs, wireframes or even just links to prior art elsewhere. Could you please elaborate?

@justafish in #18: the issue summary was updated to be more concrete, which is great, but it seems to have descoped 3 items ("we should also consider the following"): CDN support, focal point and DDoS protection. Was that the intent?

@tonypaulbarker in #24: β†’ wouldn't this steer us away from the basic premise of the issue summary, aka striving for "similar to or even reusing" Next.js' Image? The fundamental premise there is the fact that the URL is generated by the component:
https://nextjs.org/docs/app/api-reference/config/next-config-js/images#e...
Also, why is this not essentially the same as DDoS protection which @catch already talked about in detail over at #16?

@fago in #25: AFAICT https://image.nuxt.com/get-started/providers is basically the same as https://nextjs.org/docs/app/api-reference/config/next-config-js/images#e...? Both are basically "generate a URL and let whichever server that points to parse its instructions from the URL".

@effulgentsia in #26: Unsure about many things in that comment, but very strongly agreed with your opposition against adding a file ID to the "image" prop shape, and even more pleasantly surprised and intrigued by your uri-template proposal! 🀩

I don't understand yet what the proposed "group of variations" means/contains. It sounds a lot like a ResponsiveImageStyle (i.e. with a bunch of predefined ImageStyles), perhaps named DynamicResponsiveImageStyle (or ImageStyleGroup or … ), but:
without the ImageStyle config entities; instead with the contents of those config entities stored directly in this new config entity type β€” these are what determine what derivatives can be generated
with a single itok-esque anti-DDoS URL query arg value per DynamicResponsiveImageStyle, i.e. the same itok for all derivatives

But this too appears to contradict what the issue summary wrote about doing something similar to or even re-using https://nextjs.org/docs/pages/api-reference/components/image? πŸ€” How could a pure client-side component generate such a URL? It can neither know what "groups" (DynamicResponsiveImageStyles) exist, nor could it generate an appropriate itok-esque value?

Or is the idea that just like for Next images, this functionality would provide configuration options that each image-consuming component must respect β€” so that could then be:

  1. when rendering a (responsive) image using a SDC, use some Twig function we provide (in the current MR: xb_srcset())
  2. when rendering a (responsive) image using a code component, write the JS logic necessary to read + respect the settings in a JSON blob XB passes to the client side in <head>
  3. when rendering a (responsive) image using Next.js' image component, write a custom loader that contains the JS logic to read + respect the settings in a JSON blob XB passes to the client side in <head>

(That was perhaps obvious to all of you, but it wasn't to me πŸ˜‡)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

We would also like to support a client-side component similar to (or even re-using) https://nextjs.org/docs/pages/api-reference/components/image

πŸ‘† this is the issue that should have been used instead of ✨ Add automated image optimization to image component Active , but no big deal πŸ€·β€β™‚οΈ

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Too fiddly to merge today. Enjoying the sun 😎

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Odd … passes locally? πŸ€”

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@bnjmnm and I independently proposed the same at #3500795-8: [PP-2] Implement client-side validation of block settings β†’ , so let’s do it πŸ’ͺ

IMO this is borderline beta blocker, because without this, eligible block plugins (those that are fully validatable) are likely to not work correctly.
Thoughts?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Oh, great! πŸ₯³

That other issue seems to be the closer fit, so let’s do it there. I’ll point back here πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks for updating all others, I’ll take care of that last straggler! πŸ˜„

Glad to see this MR’s work coming to fruition rather than waste πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

🀞 for Ben’s theory! πŸ˜„

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Please re-test this, because it’s clearly an old version. Only test on the latest 0.x please, otherwise you’re rediscovering already fixed bugs 🫣

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Together with @bnjmnm I articulated a proposal at #3500795-8: [PP-2] Implement client-side validation of block settings β†’ 😊

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Just had a long chat with @bnjmnm about this … interesting problem space.

He had a very interesting proposal/observation/idea! πŸ˜„

What if … we don't do what this issue is proposing:

Attempt to add a translation layer from block configuration schema to json schema and send this schema for block plugins.

(which would be a huge pile of complexity!)

What if instead, we'd apply an approach similar to what we do for the (aka content entity) form? i.e.:

  • use the regular FormBuilder-powered form submission logic
  • hence use the same (PHP/server-side) validation logic (for those block plugins that do have it)

That would avoid the need for new APIs, new transformation layers, and would likely result in some shared infrastructure between the and block component forms.

Especially because the form already uses most (if not a superset!) of the #types listed at #3520484-70: [META] Production-ready ComponentSource plugins β†’ that we currently have little confidence (because no explicit tests and no block plugins exercising them) in all of those working correctly.

So … @larowlan, what do you think about exploring what it'd take to refactor \Drupal\experience_builder\Form\ComponentInputsForm to use the same logic as the tab for \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@lauriii we discussed this at length with a big group while you were on PTO. πŸ“Œ Invalid prop errors should be detailed in Component preview Active is what @effulgentsia, @bnjmnm, myself and others arrived at.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Given #3526644-9: Add xb transform to webform block storage so that it works with Experience Builder β†’ , I think we actually need something like the all-props SDC but for block plugins.

I suspect we actually need to expand what \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::checkRequirements() does, because block forms can use arbitrary render arrays, and those are definitely not guaranteed to just work in XB.

So I think BlockComponent::checkRequirements() would at minimum need to be expanded with calling ::blockForm() and traversing it to see if only "known to be working" #types are present πŸ˜…

Crediting @bnjmnm for putting #3526644 on my radar!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Note; there are lots of remaining @todos for block-sourced components β€” gathered this list by grepping https://www.drupal.org/project/experience_builder/issues/3520484#stable 🌱 [META] Production-ready ComponentSource plugins Active for "block":

(all stable blockers, not beta blockers).

Generally speaking, we've only scratched the surface with block plugins, because so few are fully validatable in core. You're likely to run into problems unless the block forms are simple at this time.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Added reference to xb transform docs thanks @penyaskito.

That's for Redux-integrated field widgets. See docs/redux-integrated-field-widgets.md.

XB's block ComponentSource doesn't use field widgets for their input UX; it uses block plugins' existing \Drupal\Core\Block\BlockPluginInterface::blockForm().

AFAICT this should not need anything else. What exactly is not working?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Great, thanks! πŸ™ I hope you'll fail, for all our sakes πŸ˜œπŸ˜„

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@mayur-sose: Can you please test + confirm that my statement in #10 is correct by manually testing as described in the issue summary? πŸ™

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Just discussed with @balintbrews β€” he's +1 to the approach here, which is good, because … I seem to have accidentally merged this as part of the MR for πŸ› JavaScriptComponent CSS libraries should depend on AssetLibrary libraries Active πŸ˜±πŸ˜‡

Marking as , to convey it's done, and it only needs that follow-up to be created. @balintbrews said he'd like to do that.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Great, </code> <code>!454 added explicit test coverage for shape matching (passing), and an e2e test (failing).

The failing test was commented out, to allow it to be worked on by somebody who knows in-depth. See #29 for the rationale.

The !1175 MR I just created uncomments the commented out e2e test coverage @thoward216 wrote and now needs a (hopefully tiny) client-side change :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

It's a core bug because if everywhere you can refer to services by their FQCN (including in *.services.yml when injecting services) except when decorating, then that's a DX WTF.

I'd then at minimum expect an example/test in core/modules/system/tests/modules/autowire_test/autowire_test.services.yml that shows that this does NOT work for decorators, and explains why.

But really, big WTF is that this has worked fine for all this time, until this issue: when used in Recipes, it suddenly breaks!?! 🀯

Test coverage like πŸ› Service decorates non-existant service when module not installed Needs work 's seems warranted.

P.S.: Seems like #3049525-5: Enable service autowiring by adding interface aliases to core service definitions β†’ and some subsequent comments are also talking about this. But not much discussion about it :/

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Big leap forward! πŸ‘

Addresses many concerns I previously raised in #26. Still some Qs on the MR though.

I'd like @bnjmnm to review the client-side transforms changes. πŸ™

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

You've commented out the entire e2e test  β€” please comment out only the bit for test_bool_default_false πŸ™

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Isn't this basically a duplicate of ✨ Add automated image optimization to image component Active ?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

How do I begin reviewing this MR? There's lots of comments here, and the issue summary doesn't talk about the client_id and clientInstanceId this MR is adding.

Can we get an up-to-date issue summary to facilitate a review? And ideally also sprinkle ℹ️ … comments over the MR to make it clearer still πŸ™

P.S.: if πŸ“Œ Document the reasons for not using Workspaces for saving in 0.2.0 Active had landed, then I'd have asked to update that ADR. In fact … any reason not to do that now, @tedbow? πŸ˜‡πŸ™ That'd be even better than an issue summary update!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ“Œ Add a Publish Experience Builder Content permission Postponed landed. πŸŽ‰

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I'd swear we recently fixed this, but I can only find πŸ› "Published" checkbox is always checked even if the entity is not Active . I bet I'm missing something.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ‘ Thanks!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The phpunit test failures are trivial expectation updates.

The back end is clearly returning (/xb/api/v0/config/component) the correct value:

        "propSources": {
            "test_bool_default_false": {
                "required": false,
                "jsonSchema": {
                    "type": "boolean"
                },
                "sourceType": "static:field_item:boolean",
                "expression": "\u2139\ufe0eboolean\u241fvalue",
                "default_values": {
                    "source": [
                        {
                            "value": false
                        }
                    ],
                    "resolved": false
                }
            },

Those 2 falses at the end are what's newly tested in that explicit prop shape example. Explicit test coverage for that seems excessive, but if others disagree, we could add it to ComponentInputsFormTest.

The new e2e test then reproduces the reported bug:

      cy:command ✘  assert	expected **<button#edit-xb-component-props-1664b908-581b-4a4c-b485-e910cb0378b8-test-bool-default-false-value.rt-reset.rt-SwitchRoot.rt-r-size-2.rt-variant-surface.form-checkbox>** to have attribute **data-state** with the value **unchecked**, but the value was **checked**

@thoward216 Can you fix the 2 PHPUnit test failures, and comment out the failing e2e test? Then we can merge this MR, and then hand this over the FE team 😊

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

That works :D

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Added some implementation guidance.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Note: spend zero seconds on making pageTitle work, until πŸ› Handle adding page title to content Active is in. Hardcode some string for now.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

wim leers β†’ changed the visibility of the branch 3529677-harden-generated-asset-libraries to hidden.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This still needed some serious cleaning up. πŸ™ˆ Did that.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I expanded the scope at πŸ“Œ Add a Publish Experience Builder Content permission Postponed to also cover the route that was just added by πŸ“Œ For selective reverting add DELETE auto-save endpoint Active under that new permission.

But … IMHO the following should also be covered by it:

experience_builder.api.auto-save.get:
  path: '/xb/api/v0/auto-saves/pending'
  defaults:
    _controller: 'Drupal\experience_builder\Controller\ApiAutoSaveController::get'
  requirements:
    _permission: 'access administration pages'
  methods: ['GET']
  options:
    _format: 'json'

πŸ‘† Will mean that you won't be able to see unless you have the necessary permissions. That will need a XB UI logic update too, to either avoid making that request at all (which would then require the UI being informed via drupalSettings.xb.permissions and hence requiring an update to ExperienceBuilderController), or to allow making that request but then NOT erroring (which would also harden against the case where the permission is revoked after the XB UI has been loaded).

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Concern: we're blanket granting all permissions in XBTestSetup. This makes it less clear which functionality requires which permissions, and makes it so that we don't end up using the extraPermissions infrastructure that exists in the e2e tests.

I'm not blocking merges for that because of the time pressure we're under, but it is this meta that is introducing many permissions, so it SHOULD (IMHO) also be up to the MRs for this meta to refine the e2e tests as needed. (In particular when testing the "test functionality when permission is not granted" case, which is true for πŸ“Œ Add a Publish Experience Builder Content permission Postponed .)

Thoughts?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I'll get this done for you :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

See #3529207-10: For selective reverting add DELETE auto-save endpoint β†’ , that just landed, and now this MR needs to be updated to also apply the new permission to that new route πŸ™

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

See #3529207-10: For selective reverting add DELETE auto-save endpoint β†’ β€” we forgot to update CONTRIBUTING.md here πŸ™ˆ My bad! Will fix there.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ“Œ Add a Publish Experience Builder Content permission Postponed will need to update this to use the appropriate permissions.

✨ Implement OAuth2 authentication for API endpoints related to code components Active made it impossible to run this test locally without also installing simple_oauth, but CONTRIBUTING.md had not yet been updated. Missed that in review there πŸ™ˆ Fixing that here.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Only +1s from @larowlan, so going ahead and RTBC'ing :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@mayur-sose You wrote "below is the one" β€” does that mean it's the only way you were able to get trapped? 🀞

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Ah, that's the detail that was missing! We've already got an explicit bug report for that: πŸ› ContentCreatorVisibleXbConfigEntityAccessControlHandler bypasses admin permissions for view Active .

Thanks, much appreciated!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

It really seems … this simple?

Because it's so simple, I'm tempted to land this without explicit tests. Doubly so because Workspaces apparently doesn't have test coverage for it either πŸ˜…

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

https://www.drupal.org/project/drupal/releases/11.2.0 β†’ was created ~7 hours ago while I was sleeping :)

So: soon "next minor" will become "current minor", at which point our CI will break unless we force CI back to considering 11.1 current minor.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ› XbConfigEntityHttpApiTest Active is in.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ› XbConfigEntityHttpApiTest Active is in. That makes it easier to adjust the tests here, which should then nicely visualize what exactly has changed.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Done by @larowlan at πŸ“Œ Improve performance of /xb/api/v0/config/component Active .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Suggested API route:

experience_builder.api.info.candidate_dynamic_prop_sources:
  path: '/xb/api/v0/info/candidate_dynamic_prop_sources/{entity_data_definition}'
  methods: [GET]
  …

So: /xb/api/v0/info/candidate_dynamic_prop_sources/entity:node:article etc.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

There's plenty more low-hanging fruit, see https://git.drupalcode.org/project/experience_builder/-/merge_requests/1....

There's also πŸ“Œ [PP-1] Refactor GeneratedFieldExplicitInputUxComponentSourceBase and FieldForComponentSuggester to need only SDC's ComponentMetadata, not SDC plugin instances Postponed to decouple this from SDCs.

But here's the real kicker: we don't need any of this until a ContentTemplate UI exists! So I've been wanting to remove dynamic_prop_sources from the /xb/api/v0/config/component response for a good while. Which is why πŸ“Œ Move `PropSourceEndpointTest` into new `XbConfigEntityHttpApiTest::testComponent()` Active has been assigned to me for months. That will simply remove these computations from the critical path!

Because the ContentTemplate UI should be able to just request the candidate dynamic prop sources on a per-content entity-t bundle basis. There's never a need to list them all across all entity types/bundles β€” I did that only for early PoC/testing purposes!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Yep, I knew this: #3515510-2: Improve backend API performance: add caching to shape matching β†’ πŸ‘ˆ that's why I moved it to the issue queue component :) This essentially does exactly what I intended πŸ˜ƒ

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#19++

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This is because you're loading XB config entities that were created using an older version of XB, which don't contain that active_version, which then triggers this assertion:

  public function __construct(array $values, $entity_type) {
    parent::__construct($values, $entity_type);
    assert($this->active_version !== NULL);
    $this->loadedVersion = $this->active_version;
  }

XB provides no update path yet β€” that will start with beta1. Right now, your only option is reinstalling. Thanks for testing our alphas though! :D

Production build 0.71.5 2024