🇺🇸United States @effulgentsia

Account created on 2 September 2006, almost 19 years ago
  • Software Engineer at Acquia 
#

Merge Requests

More

Recent comments

🇺🇸United States effulgentsia

Tagging this as beta target, because it would be nice to not have to either break BC after beta1 or support BC related to this. But not tagging it as a beta blocker, because it's not worth delaying a beta release on it.

🇺🇸United States effulgentsia

Merging the ADR is a good first step. But also, we should remove all traces of code component overrides now that we have a better solution. And best to do that before beta to not have to deal with an upgrade path for it.

🇺🇸United States effulgentsia

This will be a good performance improvement whenever we can wrap it up (looks like it's close?). It doesn't need to block a beta though.

🇺🇸United States effulgentsia

Per discussion with @lauriii, this doesn't have to block beta1, but it would be good to resolve early in the beta phase.

🇺🇸United States effulgentsia

Per discussion with @lauriii, this doesn't have to block beta1, though it would be good to fix early in the beta cycle.

🇺🇸United States effulgentsia

I discussed this with @lauriii and according to him it's sufficiently rare to have CSS that's finicky to whether the wrapper tags are there or not that he's okay with this being an area where we break BC between beta and RC.

🇺🇸United States effulgentsia

Postponing this on 🐛 PHPUnit Next Major tests failing Active which is a beta blocker. Once that's in, this issue might be quite trivial, but if it's not we're not going to hold up a beta on it. Tagging it as a beta target though so that we at least attempt to get it in if it's quick.

🇺🇸United States effulgentsia

From chatting with @balintbrews, as I understand it there's only two problems with what got merged here:

  1. 📌 Improve the way to detect if the code component has drupalSettings Active (that issue had already been opened previously, I just now re-titled it).
  2. 🐛 xbData.* libraries aren't reliably attached for use by the preview while editing a code component Active (I just now filed that issue).

Assuming it's only those two problems, let's handle them there, and I'm moving this one back to Fixed.

🇺🇸United States effulgentsia

I think what we'll want to do here is something similar to how we do first-party imports, where the XB front-end parses the JS to determine the dependencies and includes that information in the request payload when saving the component. We might need to add a new key to the js_component config schema to store the info if dependencies doesn't work for depending on Drupal libraries.

🇺🇸United States effulgentsia

"view" as the entity operation makes sense, but I think if we do that, we need to also make sure that the access handler uses a permission like "view unpublished" or "view latest version" (see Content Moderation for reference) if the entity is the one that's in auto-save.

🇺🇸United States effulgentsia

I discussed this with @lauriii and we decided that people running beta1 in production can be judicious about to whom they give the permission. It would be nice to get this resolved early in the beta cycle but we don't have to block beta1 on it.

🇺🇸United States effulgentsia

Tagging this as a beta blocker because currently in 0.x, the src property of an image prop is always set to the original image with no style applied, which means multi-MB images if uploaded from a typical camera or phone.

This issue isn't the only way to solve that problem. Other options include: providing a separate image style selector field along with the media field, or using the image style referenced by the media image type's default view mode's display, or creating an XB view mode for that. Any of those could be potentially good things to add in the future, especially to support "interesting" image styles (ones that do something other than scale an image), but the MR on this issue is close, and solves the basic use case of just wanting responsive widths in the simplest way possible from the user's perspective. Which lets us defer integrating "proper" image style configuration until we've thought through the UX of that more thoroughly.

🇺🇸United States effulgentsia

Since this was last worked on, 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active landed, so now the autosave manager may only save entities (EntityInterface objects). We could expand that to allow simple config, but I'm curious what you all think of the following idea instead...

What if we created a new entity type (neither a content entity nor a config entity, just an EntityInterface object) that can contain one or more config actions? It would have no storage but could be saved to autosave, and its save() method would instead apply the config actions.

Would that be a nice way to achieve the goal of this issue? It would have the added benefit of not having to deal with conflicts if keys in system.site other than page.front got changed outside of XB.

🇺🇸United States effulgentsia

What is srcSetCandidateTemplate?

@lauriii and I discussed this, and based on our discussion I wrote up 📌 Improve the front-end DX of Active . I don't think we should hold up this issue on that though: that issue can be a followup.

🇺🇸United States effulgentsia

Some minor DX improvements we can make to #10...

  1. srcSetCandidateTemplate is a mouthful. Perhaps this could just be srcTemplate.
  2. Although in next/image, the loader prop is always a function, perhaps next-image-standalone could allow it to be either a function or a string, and if it's a string, convert it to a function that replaces {src}, {width}, and {quality} in the string.

Doing both of the above would mean the #10 code would become:

import Image from "next-image-standalone";

const Cover = ({ image }) => {
  const {src, alt, width, height, srcTemplate} = image;
  return (
    <div>
      <Image {...{src, alt, width, height}} loader={srcTemplate} />
    </div>
  );
};

export default Cover;
🇺🇸United States effulgentsia

it doesn't make sense configuring this per instance

But where to configure it then? There's no equivalent to next.config.js for code components unless we introduce some new convention for code component global JS config. We have a global CSS tab, so maybe adding a global JS tab makes sense?

I think the ideal experience would we that we have a default loader which just works within the Drupal environment.

We could do this per #5's answer about adding defaults to config. However, the default loader would still require a per-instance prop (srcSetCandidateTemplate) set that isn't part of the next/image set of props. So #4 could be simplified to:

import Image from "next-image-standalone";

const Cover = ({ image }) => {
  const {src, alt, width, height, srcSetCandidateTemplate} = image;
  return (
    <div>
      <Image {...{src, alt, width, height, srcSetCandidateTemplate}}/>
    </div>
  );
};

export default Cover;

However, I don't think <Image {...{src, alt, width, height, srcSetCandidateTemplate}}/> is any friendlier than <Image {...{src, alt, width, height, loader}}/>.

🇺🇸United States effulgentsia

I wonder if it could be misleading that we provide something called next-image-standalone, which may make it sound like a loader function is optional, just as it is with next/image

Is it reasonable for someone to expect "standalone" (by which is meant: usable outside of Next.js) to have a way of working without providing a loader function?

🇺🇸United States effulgentsia

I prefer #5.1 over #5.2 because it's more similar to shadcn's open code approach. It makes the use of next/image (or the next-image-standalone version of it which is just a way to get next/image outside of a Next.js context) ultimately a decision made by the component author (even if we nudge that decision by providing an example), whereas to me it feels like #5.2 would imply that XB is making that decision more heavy-handedly.

🇺🇸United States effulgentsia

This is great! I wonder if the following would be an improvement or not...

What if instead of a wrapper component, Image from "@/lib/Image", people used next-image-standalone directly, but we gave them a utility function for getting the loader function to pass in? For example:

import Image from "next-image-standalone";
import {useImageLoader} from "@/lib/utils";

const Cover = ({ image }) => {
  const loader = useImageLoader(image.srcSetCandidateTemplate);
  return (
    <div>
      <Image 
        src={image.src}
        alt={image.alt}
        width={image.width}
        height={image.height}
        loader={loader}
      />
    </div>
  );
};

export default Cover;

Would the above be more idiomatic in that it would be clearer that people are using essentially the stock Nextjs image component rather than "who knows what else @/lib/Image does"? Or is the @/lib/Image wrapper idea better because it gives us room to add other customizations, like adding some defaults to the config prop?

🇺🇸United States effulgentsia

Postponing this on Unwrap astro-islands and astro-slots Active , because that issue will make this one more complicated, because once we remove the astro wrappers, it's less clear how to then target the component instance and re-render it with updated props. It's definitely solvable, just trickier.

Related, it doesn't strictly need to block a beta despite #5. Though I still stand by #5 as a strong nice-to-have, so instead of removing the "beta blocker" tag entirely, this is now our first issue that I'm tagging as a "beta target". And now that we have our first one, we'll likely start using this tag for some other issues as well :)

🇺🇸United States effulgentsia

This is currently a several second latency, just a single time and then browser cached, and only for people editing code components. It doesn't need to block a beta. It's possible we'll manage to squeeze it in before beta anyway, but we don't need to track it for that.

🇺🇸United States effulgentsia

I think this is close to landing, so whether or not it's tagged as a "beta blocker" is probably moot, but I'm untagging it per #44.

🇺🇸United States effulgentsia

I moved the "beta blocker" tag to just 📌 Create UI for selective publishing of changes Active and tagged the other children as "stable blocker".

🇺🇸United States effulgentsia

Organized the issue summary by beta blockers vs stable blockers.

🇺🇸United States effulgentsia

"alpha blocker" was a typo. This is a blocker for beta.

🇺🇸United States effulgentsia

This doesn't need to block a beta since the Page entity type doesn't have an image field (only a media field), but ideally, a stable release of XB 1.0 should work with all core field widgets.

🇺🇸United States effulgentsia

"alpha blocker" was a typo. This is a blocker for beta.

🇺🇸United States effulgentsia

"alpha blocker" was a typo. This is a blocker for beta.

🇺🇸United States effulgentsia

transforms aren't applied until after the field value validates so we might need to revisit that.

Yes, I think we need to revisit this. I'm actually surprised we've managed to get this far with XB with this existing order of operations. It seems fundamentally incorrect. We're using AJV to validate, which means our validation is based on the json schema, which means the thing we need to validate is the prop value, but we don't have a prop value until we run the transforms. Prior to running the transforms, we don't have a prop value, we only have a form input value, and form input values are allowed to be anything so long as the transforms turn them into a valid prop value.

Should we open a separate issue to fix this, or do it as part of this issue, since this issue is the first to surface the problem?

🇺🇸United States effulgentsia

I like the main concept of this MR, which is to have "dynamic image styles", which I see as embodying three things:

  • The "image style" part of the name implying that they behave just like regular image styles with respect to when, how, and where the image derivatives are generated, stored, and flushed (see further down in this comment about flushing which isn't implemented in the MR yet).
  • The "dynamic" part of the name implying that each variation (e.g., different widths) does not get saved as a separate config entity, so as not to clutter the config management and image style listing and selection UIs. The MR currently doesn't store any config entity for these dynamic image styles, but further in this comment I propose saving one for a group of variations, just not one per variation.
  • It's not implemented in this MR yet, but I think another desired meaning of "dynamic" here is that all variations within a group produce the same itok, thereby allowing the front-end to pick the variation by just varying a part of the URL without needing to also worry about getting a different itok for each one. This broadening of itok from giving access to a single derivative of a single image to giving access to a set of derivatives of a single image would make this easier to integrate into common front-end image components like the ones in Next or Nuxt. However, to prevent DDOS attacks, this means the set of variations within a group (e.g., the set of widths) needs to be reasonably bounded. This MR bounds to 8 hard-coded widths matching the default deviceSizes in Next.js. We might want this to be configurable in some way, whether we add that configurability here or in a follow-up.

With the above preamble out of the way, some thoughts about the details...

These URLs are very long.

I agree, and I don't think we need the full genericism of being able to specify any combination of effects within the image style name. I think our use-case for now is just about widths. It's conceivable we'll want additional things to be dynamic in the future (format, quality, focal point, etc.), but I don't think we need to solve for that now. I think it's equally possible that those settings don't actually need to be dynamic in the same way that width does.

In other words, I think XB's use case could be addressed with short image style names like xb--WIDTH (e.g., xb--640) or whatever other delimiter we want to pick if we think there's something preferable over --.

The dynamic image styles are cool - but if the device sizes are hard-coded - do we need them - can't we just ship N image styles in the modules config/install?

That is indeed one option, but I don't think it's optimal, because it would add clutter to image style listing and selection UIs, and image styles don't implement locked or no_ui, so we'd need to add code to prevent them from being edited or else let them be edited but figure out how to deal with them getting out of sync (e.g., their name not matching the width setting). Plus we'd still need to implement itok sharing, so they'd be in this in-between concept of sort of dynamic and sort of static. Overall, I think there's more downside than upside to have a stored config entity for each width.

This is dynamically creating the image style, but never saving it. How will it handle image style flushes when one stops being used?
Less important than flushes but also relevant here, how will it handle garbage collection when source images are deleted from disk?

One option would be for XB to implement hook_cache_flush(), hook_file_move(), and hook_file_predelete(). However, that wouldn't exactly provide parity with regular image styles, because regular image styles are not flushed by a regular cache clear and can be flushed on their own via the UI or a Drush command. So if we want parity with that, I propose that we create an image style that represents the group. For example, we ship with an xb image style, which then exists as a stored config entity, that xb--* image styles are "derived" from (but not individually stored). Then we can implement hook_image_style_flush() to flush all the --* ones whenever the base one is flushed.

"target_id": {
  "title": "file id",
  "type": "integer"
}

I don't like this being added to the json schema definition of the Image prop type. Prop types should be independent from Drupal's data model. What I recommend instead of this is adding a srcsetTemplate property that's of type uri-template. So its value for an image foo.jpg would be /sites/default/files/styles/xb--{width}/public/2025-06/foo.jpg.webp?itok=.... Client code could then generate a srcsset from this by replacing {width}. Or, if integrating with a Next or Nuxt image component, you'd configure a loader/provider function that does that replacement.

Ensure that the module can function with security tokens (i.e. itok)

Per the top of this comment, we need the same itok for each width. This MR already implements a param converter that instantiates the image style even though it's a dynamic image style that doesn't exist in config storage. I think the only addition needed for this is to instead of instantiating it as an ImageStyle to instantiate a subclass (e.g., DynamicImageStyle), where the subclass overrides getPathToken() from hashing $this->id() to instead hashing just the part of $this->id() before --.

🇺🇸United States effulgentsia

We're going to have some early XB adopters at Acquia (and maybe elsewhere, but I only know about the ones at Acquia) running XB beta1 (which is great for helping us find problems ahead of tagging a stable release) who won't be able to upgrade to Drupal 11.2 right away, so we're not quite ready yet to constrain XB to 11.2 only.

🇺🇸United States effulgentsia

I haven't looked at the XB codebase related to this deep enough to comment on the code-level implementation suggestions of #8, but I'm +1 to the high level premise if I understand it correctly, which is:

If an auto-saved entry exists, and there is 1 or more fields for which:

  • The value is different than what the published entity has, and
  • The current user doesn't have edit permission for it

Then the user should not be allowed to make any changes at all on the Page Data tab. I think ideally the Page Data tab would still be visible, but be read-only, with a message at the top saying that it's read-only because there exist unpublished changes to it that were made by someone with different permissions and must be published or reverted by someone with those permissions before it can be editable by others.

Then we can punt any further scope (i.e., allowing edits of the permissible fields and merging those into the auto-save) to a post-beta, and maybe even post-stable, follow-up.

🇺🇸United States effulgentsia

Any downside to doing a version check?

'sqlite_type' => version_compare(\Drupal::VERSION, '11.2', '>=') ? 'json' : 'text'

I'm not clear if any upgrade path would actually be needed or if the above would just automatically make things better once people upgrade to 11.2.

🇺🇸United States effulgentsia

With all of the other changes that have landed in the last few weeks, is this issue still relevant? If so, what are the steps to create a broken auto-save state?

Tagging as a beta blocker to answer this question and evaluate. Depending on the answer, we might decide that actually fixing it doesn't necessarily have to block a beta.

🇺🇸United States effulgentsia

It's possible this is already at a state that's good enough for beta, but in case it isn't I'm tagging this as a beta blocker to make sure we evaluate it before releasing a beta.

🇺🇸United States effulgentsia

Possibly related to #16, if Component config entities have the config schema type of versioned_config_entity, what happens if an old version has a dependency but the active version doesn't have that dependency? Does the dependencies of the Component config entity include only the dependencies for the active version, and if so, is there anywhere that tracks the dependencies of old versions (for which there might still be stored json blobs)?

🇺🇸United States effulgentsia

I'm not quite following the last two paragraphs of #15. What plugins do the JSON blobs for StaticPropSources depend on that aren't dependencies of the Component config entities?

🇺🇸United States effulgentsia

This MR doesn't need the Drupal\Driver\Database\sqlite classes, only the Drupal\experience_builder\... classes. The former was only for Drupal 9 and maybe early minor releases of 10. https://www.drupal.org/project/mysql57 and https://www.drupal.org/project/sqlite337 can be used for reference for Drupal 11.

drush site-install doesn't work if I have the namespace

Yeah, I've run into this too with https://www.drupal.org/project/sqlite337 when installing without an already existing settings.php. What I found is I need to use the browser installer to write out settings.php the first time, and then drush site-install works if I keep that settings.php around.

Stepping back a bit though, I'm curious why XB needs to leap ahead of 🐛 Cannot delete a field which uses JSON type Active . Does SQLite itself actually have a json type? I thought it only had text and jsonb, and you can migrate from text to jsonb both incrementally and at anytime, so why does XB need to do it before core's driver supports it?

🇺🇸United States effulgentsia

Also, we don't need these when the user first enters the code editor, do we? Since either it's a new code component, so we're using the sample code, or it's an exiting code component, so we're using whatever was previously saved, so in both cases, we already have previously compiled stuff we can show in the preview. Which means we don't need to execute the wasm code until the user starts typing something into the code editor. At which point, could we put a message in the preview area saying "Generating Preview..." or something like that if the wasm files haven't fully downloaded yet?

🇺🇸United States effulgentsia

The code in the MR looks good, so I approved it (before seeing Wim's comments), but it seems questionable to me to be preloading 30MB of stuff (even with low fetch priority) before we even know the person wants to edit a code component. I wonder if we should delay this until there's some further indication of that, such as:

  • Clicking "Add new component" (after which there's a bit of time for the user to enter a name for it)
  • There already existing at least one code component (either as a real config entity or in auto-save)

React-dom has a preload function by the way, if we want to initiate this based on some client-side determined condition.

Since this MR only preloads for people with the "administer code component" permission, I'd be okay with this being merged as-is (once Wim's okay with it) and us discussing the above in a follow-up.

🇺🇸United States effulgentsia

Although I don't think this needs to be figured out before beta, I think before tagging a stable release we should come to a decision on whether a slot should be thought of as a black box, or if it's legitimate for SDCs to treat a slot as an array of components and be able to decorate them in some way.

🇺🇸United States effulgentsia

Tagging this as a beta blocker, because we want early adopters of the beta able to run XB in production, including under potentially heavy server load.

🇺🇸United States effulgentsia

Upgrading this to a beta blocker, because we want early adopters of the beta able to run XB in production without having their CSS break when we eventually add SSR.

🇺🇸United States effulgentsia

Editing the JS code of code components requires a restrict access permission, so using non-sandboxed iframes for the various previews isn't a vulnerability, but sandboxing them would help add extra defense against some privilege escalation vectors, so switching the tag from Security to "Security improvements".

However, I'm still tagging this as a beta blocker as well, because we want early adopters able to run the beta in production, and this would help provide extra confidence for doing so.

🇺🇸United States effulgentsia

Yay about #5 that Views across exposed slots is not a necessary capability.

Another concern I thought of: if we do one-field-per-exposed-slot, would that prevent us from being able to use Workspaces and wse_config to stage changes to content templates if such a change involves adding or deleting exposed slots? XB doesn't yet integrate with Workspaces and wse_config, but it's something we'd like to add eventually, even if after 1.0.

Production build 0.71.5 2024