Massachusetts
Account created on 15 November 2007, almost 18 years ago
  • Senior Software Engineer at AcquiaΒ 
#

Merge Requests

More

Recent comments

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merge train running.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This should not be postponed, now I think about it. This is Canvas outright breaking Webform, content templates or no content templates.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

It's an API addition so yeah, probably needs a CR.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

If you got past it, great! I don't see how this could have been caused by Drupal CMS itself but feel free to reopen if you think there's something we can do here.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I'll give that a try, but I'm honestly not sure how that could have happened. Drupal CMS does not include entity_reference_revisions -- at least, not as a direct dependency -- and never has. Maybe one of our dependencies added a dependency on it.

But I'll certainly give it a shot from scratch and see if I run into this.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Unfortunately, 2.x-dev is not releasable at all; it's only there as a ZIP file because drupal.org's packaging system requires it.

Sit tight -- we expect to have an alpha available around DrupalCon Vienna!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This broke HEAD; unit tests are not passing because of the deprecation. The follow-up MR needs to be merged tout-de-suite.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Added an event subscriber to do for the core API what Default Content 2.x does here: https://git.drupalcode.org/project/default_content/-/blob/2.0.x/src/Norm...

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

@lauriii explained this issue to me on Zoom. Basically: optional properties (anything not explicitly defined as required) should be allowed to default to null.

I don't know what the correct fix is, and I have no opinion on that. But you're going to need test coverage of whatever that fix is, so self-assigning to write a test.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Well, this did uncover an actual flaw in Recipe Installer Kit, but that is also good to know!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Project Browser did change its configuration format, but Drupal CMS adjusted to it πŸ› Our default configuration for Project Browser is no longer valid Active , long since released.

Can you provide full steps to reproduce, including the commands you ran, starting from scratch?

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Traced this to a bug (sorta) in Recipe Installer Kit.

The problem is that our install profile's info file lists drupal_cms_starter as a required recipe, so it's always applied. But the site template also applies it. It gets applied twice, effectively, so not only is that a performance ding, but the config action runs twice and adds the blocks to the dashboard layout twice.

The quick fix here is to remove the list of required recipes from the install profile. The downside of this is that, if you don't choose a site template at all, you get a completely empty Drupal (or worse). Recipe Installer Kit has no concept of a fallback site template.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

phenaproxima β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

...I just realized that I need to also do this for rsync.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

And, merged into 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

My stop-gap measure has been to create a computed url field for nodes which returns the canonical URL of the node: https://git.drupalcode.org/project/drupal_cms/-/blob/2.x/drupal_cms_help... and https://git.drupalcode.org/project/drupal_cms/-/blob/2.x/drupal_cms_help....

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merge train started for 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Committed directly to 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x and partially cherry-picked to 1.2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

phenaproxima β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merge train started. Destination, 1.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merge train started!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Hmmm...if those things aren't used anywhere, then they should probably be removed instead of codified in config schema! That'd need an update path, I'm betting.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

phenaproxima β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merge train started!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

phenaproxima β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Made one minor change but otherwise I think this is great.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x and cherry-picked to 1.2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This is no longer a Drupal CMS critical, because we are working around it in our helper module -- for Webform, at least, which is okay for the time being. It would be great to fix it before stable, but if that doesn't happen, Drupal CMS will not be broken by it. Untagging.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This is no longer a Drupal CMS critical, because it's only needed for developers who are trying to build site templates using Canvas, and they (including us!) can patch Canvas accordingly while developing.

It would be great to fix it before stable, but if that doesn't happen, Drupal CMS will not be broken by it. Untagging.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This is no longer a Drupal CMS critical, because we are working around it in our helper module. It would be great to fix it before stable, but if that doesn't happen, Drupal CMS will not be broken by it. Untagging.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

With Pam's help I managed to trace this. We has found the problem and they is us.

The problem is that Drupal CMS 1.x still requires the drupal/drupal_cms_analytics metapackage, which is a metapackage and therefore untouched by the recipe unpacker. The problem with that, is that this metapackage...requires recipes! So this ends up reintroducing in the sandbox what the recipe unpacker has already removed from the live code base, triggering the Package Manager validator -- which is absolutely doing the right thing here.

The correct solution, in this case, is to get rid of drupal/drupal_cms_analytics. For context, this package was created before 1.0, when it became apparent that we wanted to scope the analytics recipe more tightly to just Google Analytics. Due to a timing snafu, it was released with this wrong, generalized name (and might have even gone to beta with that name -- I don't recall exactly). We couldn't rename it, having already published it, so we did the next best thing, which is publish the drupal/drupal_cms_google_analytics recipe alongside it, and make drupal/drupal_cms_analytics a metapackage containing it and, presumably at the time, any future analytics recipes.

Welp, I think it's clear now that the existence of that package was a blunder on our part. Would probably have been better for drupal/drupal_cms_google_analytics to replace drupal/drupal_cms_analytics (using the replace key in composer.json), but you know what they say about hindsight.

So, my plan: in another issue:

  • Remove drupal/drupal_cms_analytics from our subtree split and from the project template's composer.json, replacing it with drupal/drupal_cms_google_analytics. This will happen in both the 2.x and 1.2.x branches.
  • Alter drupal_cms_analytics to allow any version of drupal_cms_google_analytics.
  • Mark the drupal/drupal_cms_analytics project as obsolete. It will never receive another release.
  • Roll a new release of drupal_cms_analytics and drupal/cms which contains this change.

Here's what I think the effect will be:

  • New sites: they'll never know that drupal_cms_analytics ever existed. They'll get drupal_cms_google_analytics and unpack it happily.
  • Existing sites: although the continued presence of the metapackage is not harmful to the operation of the Drupal site, it could cause trouble for Composer (as we've seen in this issue). We will have a change record which recommends unpacking all recipes (if it hasn't been done already) and removing the metapackage, with instructions on how to do that.
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This was all done in πŸ“Œ Remove the ability to configure the path to Composer Active , it seems, so closing it as a duplicate.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

The idea -- and I probably should have documented this in the issue summary, or open a postponed follow-up -- is that in Drupal 12, we'll have an update path to remove those keys.

So we commit this now, and give people a nice, long runway to make the requisite changes. The change record documents how to do it. But if people don't remove the keys, they will still only get requirements warnings, not errors, and if they're okay with that, then the keys will automatically go away when they update to Drupal 12.

Does that seem reasonable? Tentatively restoring RTBC here since I think that's what we'd kinda-sorta agreed on in Slack discussions, but by all means kick this back if I'm jumping the gun here. (Or, if you want me to open a follow-up for the upgrade path that removes the keys.)

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Started the merge train for this.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I think a README.md in each recipe makes a ton of sense! That's easy, and it keeps the "source of truth" in one place. d.o has an easy function to copy README.md into the project description, so that's an even better reason to go with that.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merge train started.

✨ | Drupal core | [PP-1]
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I think that would make sense to do in a follow-up.

✨ | Drupal core | [PP-1]
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Changing status per #36.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

We could do this, but the only problem is that we can easily add (or remove!) a module from a recipe and forget to update the project page. If we could have links to some part of MODULES.md which lists the modules, that would be better.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Committed to 1.x!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

We need to move this patch to the Olivero project because it’s been removed from our subtree split.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Fixed both suggestions, which are minor enough that I think we can go directly back to RTBC here.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Thanks @joelpittet! This is a nice improvement. Merge train started!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

The merge train has now left the station! Should be committed shortly when all tests pass.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I'm going to merge this to get people unblocked, even though the bug itself is due to preexisting incorrect pattern -- you are never supposed to inject a config object into a form (or any class) as a dependency.

The correct pattern would be for the form to override the getEditableConfigNames() method, and use $this->config() internally to load config. Core will guarantee that it gets editable versions of anything listed in getEditableConfigNames().

Merging this on the good-faith assumption that some kindly person will open a follow-up issue in which to do that cleanup.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

I want to just update here after the discussion and decisions that took place in πŸ“Œ Try to fork Mercury on install of a site template Active (and, honestly, a lot of Zoom calls and Slack channels, including DMs).

Ideally, Canvas (or some other page builder) could handle the theming for a site template, pretty much from top to bottom. That's the ideal future state. But Canvas isn't there yet. There are still many rough edges and missing features.

So, for the time being, we couldn't escape the conclusion that we need some way for site templates to ship a theme.

@lauriii and @pameeela felt strongly (and I agree, for the record) that we don't want to rely on runtime theme extension (i.e., base theme) because the DX of finding out what is overriding what, and where, is excruciating if you're not a) a developer and b) used to Drupal's way of doing things. So we are instead embracing the starter kit concept that has been supported since Drupal 9.3 with the introduction of the generate-theme console command, and the starterkit_theme theme.

Details can be found in the issue I just linked (the final comment I made before merging the MR), but we will have two ways for site templates to include a custom theme -- generate it, or include it and make Composer automatically move it to /web/themes/custom. In either case, the outcome is the same: the site's theme is one theme (no runtime extension), and it is completely under the site builder's control -- it does not extend another theme, or receive updates. If a site template ships or generates a theme, that theme is treated as a custom theme, full stop.

This is not a hard-and-fast rule for all site templates; they can manage their look and feel however they like. But this is the path that Drupal CMS has chosen for its initial round of site templates and, until Canvas has all the features we need, probably the for-now best practice.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

OK, so here's where we landed, with agreement from Lauri (here and on Slack/Zoom) and Jason and Lanny. This is what I'm merging.

A site template can do either, or both (or none) of these:

  • Ask Composer to generate a custom theme on its behalf, based on some starter kit it specifies in composer.json. This is what drupal_cms_starter does, generating a Mercury fork called starter_theme (we can rename this later if we want to). The idea is that, if you don't start with a site template, you start with a blank slate and a detached theme, based on but not yoked to Mercury, that you can customize freely.
  • Include a custom theme, in a magically named theme subdirectory. If this exists, the machine name of the theme will be inferred from the info file, and the theme will be moved, not copied, into the themes/custom directory for further customization, if desired. This custom theme can be a completely custom theme and doesn't have to be based on Mercury or any other starter kit.

In either case, the site builder is the person who ultimately has complete control of the theme, which is considered (and treated) as a totally custom theme.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

What if a site template wants to include an admin theme, or multiple themes (domain access, multisite, etc)?

It can bring those in as normal Composer dependencies.

Now I have to have a place to work on the theme, and then zip it up, or create a deployment mechanism to do package things up instead of a normal git composer workflow.

Maybe it's a theme directory instead of a zip file, then. Not sure yet. But the overall gist is that the theme (since we unfortunately need to have one) should be embedded in the site template somehow.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

@catch directed me to πŸ“Œ Use a better container cache key Active , which would also greatly mitigate this problem.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

In addition to #3, we can also put the cache clear block (provided by core) into the dashboard, so users don't have to go digging around for it.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

The technical underpinnings for the form itself have been implemented in Recipe Installer Kit, and can be seen if you clone the 2.x branch and run ddev launch to get into the installer.

It doesn't look great, but that's only because we are lacking design!

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

phenaproxima β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

phenaproxima β†’ made their first commit to this issue’s fork.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

There is one thing I'm wondering as I look at this -- I wonder why adding a module somehow decided to bring in all of those recipes, which are clearly unpacked (and that's why they're not Composer-managed). But why would they get added anew?

Can you provide exact steps to reproduce this locally? After setting up Drupal CMS, what module did you try to add and how did you go about it?

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

This is coming squarely from Package Manager: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/modules/packa...

When this validator was written, we had no concept of recipes or recipe unpacking. This is a clear bug where Package Manager needs to be altered. A few ideas:

  • Remove the validator entirely
  • Convert the error to a warning
  • Don't do the validation for packages that are recipes, since we know they could be unpacked

In the meantime, Drupal CMS's helper module can and should work around it by disabling the validator (and, for that matter, by also implementing the wait_timeout workaround).

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

phenaproxima β†’ created an issue.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Capturing some discussion with @pameeela in Slack - ideally a site template would not need to rely on a bespoke theme at all; it would be able to just spin up with an automatic fork of Mercury (so that Mercury is free to evolve as it needs), and then do all further customization in Canvas, with JS components being used if custom design elements that stand outside Mercury's design system are needed.

Canvas isn't quite there yet, though. Site templates still need custom themes, based on Mercury but not extending it (i.e., themes forked from Mercury as a starter kit, rather than using it as a base theme), that can implement their customized styling or tweaks.

Ideally, they can ship these themes in such a way that they immediately become a part of the site itself, rather than exist as specific projects published on drupal.org. That will lead directly to a wasteland of dead and unmaintained themes -- a problem we would like to avoid.

My proposal is that we use Site Template Helper to support the following workflow:

  • A developer creating a site template uses core's generate-theme command to create a custom theme that is a renamed Mercury clone.
  • They build their site template on that theme, customizing it as needed.
  • When it comes time to distribute the site template, they make it depend on Site Template Helper, and they zip up the theme into a file called theme.zip, that sits in the site template.

When a site builder spins up a site using that template:

  • The site template is downloaded and installed by Composer, just like any other recipe.
  • Site Template Helper, being a Composer plugin, springs into action and extracts the theme.zip file to themes/custom.
  • The template can then be applied like a normal recipe. The theme is completely disconnected from Composer, and it is added to the project's Git repository as a totally custom, bespoke theme that will not be affected by any upstream changes in Mercury.

Although this isn't necessarily the ideal long-term solution, it solves the problem of depending on Mercury (or any other upstream theme), and it solves the problem of spamming drupal.org with highly custom, one-off themes, meant for but disconnected from specific site templates. It unambiguously puts the site's presentation layer under the complete control of the site template user, and the theme is truly the only thing that is controlling the site's look and feel. It's also a fairly easy workflow for site template developers -- "generate a theme and stuff it in a zip file" is simple to grok.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Created https://www.drupal.org/project/site_template_helper β†’ for this, at @alexpott's suggestion. It's really only meant for site templates. We shouldn't use it yet because we haven't yet figured out what the best practice is.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Merged into 2.x. Not backporting because:

  • ai_agents has no stable 1.2.x release
  • The existing constraints in Drupal CMS 1.2.x will not block updating either module, so this version bump is really just housekeeping, not a necessity
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts
πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

Auto-merged into 2.x. I'll have to backport it to 1.2.x.

πŸ‡ΊπŸ‡ΈUnited States phenaproxima Massachusetts

phenaproxima β†’ created an issue.

Production build 0.71.5 2024