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

I think we might want to postpone this one on the release of Drupal 11.3, because that version of core has Recipes should support skipping config actions on entities that don't exist Active , which would absolutely provide a clear path for us to do this optional integration. Unfortunately, new recipe syntax is not something we're able to polyfill in our helper module, so if this is not a stable-blocking issue for Drupal CMS, it should wait.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

The proposed division of labor here, after discussing with @fjgarlin at DrupalCon Vienna, is for Fran to a new opt-in job to the DA's GitLab CI templates which:

  1. Creates a stable Drupal CMS project
  2. Adds the module/theme/recipe under test to it
  3. Copies a generic test, which we supply (but that the module/theme/recipe under test can override and customize) which confirms that said extension is installable on Drupal CMS without errors

The first two things are for Fran; I will handle the third part in this issue.

🇺🇸United States phenaproxima Massachusetts

Briefly discussed with @lauriii, @f.mazeikis, and Gábor at DrupalCon Vienna. Our current best guess is that this is a Canvas issue; we are unsure if Canvas has the ability to shape match the Tags field, since it's an infinite-cardinality entity reference.

Wim patiently explained the internals of prop expressions and shape matching to me today (so I'm crediting him as well), and I feel confident about being able to dive more deeply into why this isn't working. If it's a Canvas bug, we'll transfer this issue to Canvas' queue and fix the bug there. But for now, I'm best equipped to look into it from the Drupal CMS perspective, so assigning to myself.

🇺🇸United States phenaproxima Massachusetts

Discussed with @pameeela and @emma-horrell at DrupalCon Vienna, and they confirmed that there is virtually no reason why a creation timestamp should ever be included with default content -- the net effect is confusing and unhelpful. If someone wanted to include a creation time, for some reason, they could do it manually (or implement their own code for it).

This should be implemented in the helper module's \Drupal\drupal_cms_helper\EventSubscriber\DefaultContentSubscriber class. In the preExport method, the entity being exported should be scanned for fields of the created type, and those fields should be marked exportable. Core has an example of this; see https://git.drupalcode.org/project/drupal/-/blob/11.x/core/lib/Drupal/Co...

Additionally, we'll want to remove the created field from all existing exported nodes in our recipes. I don't think this will need explicit automated test coverage; a little manual testing, by exporting a node that a recipe creates and confirming that the created field is not there, should suffice.

With that all outlined, I'm going to leave this for someone else to implement -- if any devs would like to contribute to Drupal CMS, this could be a nice, fairly easy win!

🇺🇸United States phenaproxima Massachusetts

A few points, but I really like where this is going and it's a great idea. I gotta say, this is also a pretty damn decent use of AI.

  1. There are unexpected line breaks in the Markdown files, can we have the prompt eliminate those? (This is relatively minor, as the line breaks do not appear to affect how the READMEs are displayed in GitLab, but may prove problematic when the contents are transferred into drupal.org project pages.)
  2. Can you add a section to each README, before the Contributing section, about what do if you encounter bugs or problems, and where to go? (The Drupal CMS issue queue, naturally.)
  3. The fact that core modules are not linked makes sense, but IMHO it feels weird. Maybe just let those link to core's page at https://drupal.org/project/drupal? (We could also maybe list them like Navigation (core) so it's clear that these are core modules.) It would also be nice to do the same for applied recipes so that it's consistent, and core recipes can always be detected because they will be prefixed with core/recipes/.
  4. Minor formatting thing, but the Composer command to install the recipe should ideally be a block, which in Markdown is signified by three backticks (```) before and after, on their own lines.
  5. Let's link "Drupal CMS" (in the Installation section) to https://drupal.org/project/cms, to keep things in line with the projects' current pages.
  6. .

  7. Drupal CMS Helper is left out in the cold and doesn't get a README.md; it would be nice if it did!
🇺🇸United States phenaproxima Massachusetts

This seems reasonable to me, with solid test coverage, but I have a few questions/suggestions.

🇺🇸United States phenaproxima Massachusetts

This looks really good to me, but I suspect we can tighten scope slightly and reduce the size of the change set.

🇺🇸United States phenaproxima Massachusetts

Tagging for backport to the 1.2.x branch.

🇺🇸United States phenaproxima Massachusetts

I think this is a great start and a great idea. Left a few comments.

🇺🇸United States phenaproxima Massachusetts

So to be clear --

We should disable "Clear filters" if all filters are in the "cleared" state.
And we should disable the "Recommended filters" if all filters are in their default (recommended) state. And we also shouldn't show it if all filters' default state is the same as their cleared state.

This is what came out of discussion in Vienna with @ckrina and @pameeela.

🇺🇸United States phenaproxima Massachusetts

Done-zo.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

Shippety-ship it.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Wow, this took a while but it's a nice fix. Merge train into 2.x started. Thanks!

🇺🇸United States phenaproxima Massachusetts

Discussed with @alexpott and we agreed to move the question mark to the beginning of the config name, not the end. That matches PHP nullable parameters and Symfony's optional container services.

🇺🇸United States phenaproxima Massachusetts

Couple of minor things.

🇺🇸United States phenaproxima Massachusetts

'Tis perfect. Thank you! Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts

The blocker is fixed, and more importantly released, so this should be easy enough to do in the drupal_cms_starter recipe, which is what disables the recipes source in the first place. The fix will look something like this:

config:
  actions:
    canvas.component.block.project_browser_block.recipes:
      disable: []

(Plus a slight adjustment to Drupal CMS Starter's ComponentValidationTest to confirm that this did what we expected. I can handle that part if whoever implements this isn't comfortable writing tests, but in all honesty, it should just be a one-line change here: https://git.drupalcode.org/project/drupal_cms/-/blob/2.x/recipes/drupal_...)

Tagging as a novice issue for contrib day at Vienna.

🇺🇸United States phenaproxima Massachusetts

Was re-reviewing this and I noticed a couple of things missing:

First, is it possible to invoke these actions with a non-array properly? Example:

simpleConfigArray:push:
  key: whatever
  values: foo

It should be. We need test coverage that this doesn't blow up.

Secondly, do we have multiple invocations support? As in, what if we need to manipulate two arrays in the same config object? Example:

simpleConfigArray:push:
  - key: something
    values: [a, b]
  - key: something_else
    values: [c, d]

If we don't, we need this too, and test coverage to accompany.

🇺🇸United States phenaproxima Massachusetts

Thanks for the patch! This was hotfixed in v1.2.8.

🇺🇸United States phenaproxima Massachusetts

Reopening to create an MR for 2.0.x, which still fails the stylelint job.

🇺🇸United States phenaproxima Massachusetts

I figure I'll write the tests on another MR in this issue's fork.

🇺🇸United States phenaproxima Massachusetts

This should probably be backported. I suspect it will be a clean cherry-pick.

🇺🇸United States phenaproxima Massachusetts

Straightforward and tested.

🇺🇸United States phenaproxima Massachusetts

Merged !84 into 1.x.

🇺🇸United States phenaproxima Massachusetts

@alexpott and @mxr576 walked my malcaffeinated brain through this at DrupalCon Vienna and now I get it! For a beautiful moment.

This bug fix is legitimate and the test coverage makes sense. The guardrail against collecting recipe input twice is still there, it's just less aggressive. That makes sense. Ship it.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

One request, otherwise RTBC.

🇺🇸United States phenaproxima Massachusetts

Couple of minor points but damn this looks good. We should get it committed post-haste.

🇺🇸United States phenaproxima Massachusetts

I think we can make the metadata read-only and therefore remove some accessors from the event.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue. See original summary .

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

I think the solution here is in Project Browser; blocks should not be derived based on configuration settings.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

Merged into 1.x.

🇺🇸United States phenaproxima Massachusetts

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

🇺🇸United States phenaproxima Massachusetts

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

🇺🇸United States phenaproxima Massachusetts

OKed by @galactus86 in person and merged into 1.x.

🇺🇸United States phenaproxima Massachusetts

Merged into 1.x, with @galactus86's in-person approval, after locally running pnpm run build and discovering that no changes were made to the built CSS.

🇺🇸United States phenaproxima Massachusetts

We probably should refactor our ECA content cloning model so that it keeps values in private tempstore rather than passing them in the URL. That would probably get us past this.

🇺🇸United States phenaproxima Massachusetts

@catch, is this still something we want?

🇺🇸United States phenaproxima Massachusetts

All Cypress tests were removed in favor of PHPUnit and whaddaya know...the flakiness utterly ceased.

Closing as outdated.

🇺🇸United States phenaproxima Massachusetts

Searched the code base and I can't find any examples of simpleConfigUpdate being used on a config entity anywhere in our recipes. Closing this as outdated.

🇺🇸United States phenaproxima Massachusetts

As of Recipe Installer Kit 1.0.0-alpha8, this is possible by adding this to the recipe:

extra:
  recipe_installer_kit:
    finish_url: '/some/path'

...and both Byte and Starter are using it.

Closing this one as outdated.

🇺🇸United States phenaproxima Massachusetts

I'm told from @tim.plunkett (Form API maintainer) that is best not to change the #action unless you have specific reason to do so. Simple Search Form does, but it should really only do it if an action path is set. Slightly changing the MR to account for this.

🇺🇸United States phenaproxima Massachusetts

Created an MR with proposed quick-fix.

🇺🇸United States phenaproxima Massachusetts

I would have to guess this is a bug with Simple Search Form. The line in question:

$form['#action'] = Url::fromUserInput($config['action_path'])->toString();

It's totally unclear where $config would be coming from, but it's an additional non-standard (i.e., not defined in the interface) argument to the buildForm() method.

To me, that probably indicates a bug in Simple Search Form itself; it never accounts for the possibility that $config is not passed in. But the severity of the problem marks this as a Drupal CMS stable blocker, which is therefore also a critical.

🇺🇸United States phenaproxima Massachusetts

I was in Vienna for DrupalCon while reviewing this, so tagging accordingly.

🇺🇸United States phenaproxima Massachusetts

This seems like an excellent start and it simplifies a few things. I have a few questions about the code, and some relatively minor (so far) review comments. It will need a more in-depth review overall.

We'll need dedicated test coverage, so tagging for that.

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x from the passport control line in Lisbon’s airport at 6 in the damn morning after not getting any sleep on the plane. Thanks for pushing this over the line, @penyaskito.

🇺🇸United States phenaproxima Massachusetts

@penyaskito has generously offered to handle this since I may be in the air when Canvas RC1 is released. Assigning to him.

🇺🇸United States phenaproxima Massachusetts

Merge train to 2.x started. Thanks for the monumental effort here, y'all -- feels great to finally ship this.

🇺🇸United States phenaproxima Massachusetts

ECA committed all blockers and released! This is no longer blocked; indeed, tests of it should be passing.

🇺🇸United States phenaproxima Massachusetts

I wonder if this could dovetail with the work being done to make config actions translatable? The current approach proposes adding a !translate YAML tag, which presumably would allow potx to find the strings that need to be translated for a project (which would include recipes and, potentially, themes that expose SDCs).

🇺🇸United States phenaproxima Massachusetts

Postponed on the release of Canvas 1.0.0-rc1, expected later today. Self-assigning to merge this when that drops.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

Replaced mentions of XB with Canvas.

🇺🇸United States phenaproxima Massachusetts

Okay, responding to the points #24:

  1. I tried uncommenting that, and it broke tests in very strange ways that, to me, are indecipherable. I assume this is what you were referring to when you said "I am probably the only one who can [update our shape matching logic]". So I left that as-is.
  2. I opened #3551455: HostEntityUrlPropSource should be able to support absolute or relative URLs, URL options, and different link templates to robust-ify and round out the capabilities of HostEntityUrlPropSource, and updated the @todos to refer to it. You are correct that what is in 1.x HEAD right now is enough to unblock Drupal CMS and remove any need for us to ship a computed url field as a shim -- for which we are very grateful!
  3. Made the doc updates you mentioned. I'm not sure if I missed anything, or if there's any docs that I'm unaware of which also need to be updated. Happy to do that if you point out where I should look.
  4. Updated as you suggested.
  5. Ditto.
  6. Ditto.

I'm assigning to you to handle the shape matching piece of it, which will presumably also need tests and documentation, so leaving those issue tags in place.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

Thank you! This is a nice solution.

🇺🇸United States phenaproxima Massachusetts

I can confirm that this does exactly what is intended; I tested it out in the context of Drupal CMS 2.x's card content template for blog posts, which is where we're currently using the computed url field as a shim.

🇺🇸United States phenaproxima Massachusetts

Never mind, just randoms.

🇺🇸United States phenaproxima Massachusetts

Hmmm, tests for drupal_cms_starter seem to be legitimately failing.

🇺🇸United States phenaproxima Massachusetts

Started a merge train to 2.x.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

@pameeela and @tim.plunkett strongly signed off on this verbally, and tests are passing. In it goes. Merged into 2.x and cherry-picked cleanly to 1.2.x so that Drupal CMS users can benefit from this right now.

🇺🇸United States phenaproxima Massachusetts

Does this need the main CSS rebuilt?

🇺🇸United States phenaproxima Massachusetts

Linking a well-known core bug which affects recipes that ship translation languages.

🇺🇸United States phenaproxima Massachusetts

Question about the proposed fix here:

  • Why is there a need to return the number of languages changed? That seems superfluous and is adding complexity to the patch.
  • Could we not just fix this by using the null-safe operator when trying to load the configurable language that may or may not exist? Surely that would get us past this problem elegantly, and would fix the flawed assumption which is causing the bug (namely, that the Language module is assuming that all of its config actually exists, which is not and never has been a safe assumption).
🇺🇸United States phenaproxima Massachusetts

This bug can prevent recipes from shipping translations without adding the following workaround to language entities:

dependencies:
  config:
    - language.entity.und
    - language.entity.zxx

Tagging, therefore, as part of the Recipes Initiative.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

I've attached a zip file that is (with some minor alterations to work around a couple of known core bugs and quirks of the Umami profile) the output of the drush site:export command, run against Umami.

How to test this?

  1. Spin up Drupal CMS 2.x -- no need to check out this MR branch.
  2. Unzip the zip file so that it is extracted at recipes/oomami.
  3. Go into the installer (ddev drush sql:drop -y && ddev launch)
  4. You will see the Oomami site template as an option. Select it, and proceed through the install process.

When the install completes, you'll be redirected to a 404. This is due to a quirk/bug of Recipe Installer Kit which I need to fix separately.

But otherwise, it's 100% Umami. Fully reconstituted as a recipe. No install profile involved.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Added Facilitate site template development by hooking into config exports that target a recipe Active as a related issue, where I propose adding some greased-slide tooling to Drupal CMS to help site template creators.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

This works as intended!

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

I'll get to the IS update later.

🇺🇸United States phenaproxima Massachusetts

One suggestion for clarity, RTBC otherwise.

🇺🇸United States phenaproxima Massachusetts

I'm not sure the %bundle replacement in keys even exists in 10.x at all, but if it does, then IMHO it should be backported.

🇺🇸United States phenaproxima Massachusetts

phenaproxima created an issue.

🇺🇸United States phenaproxima Massachusetts

I had originally implemented this as an adapter in Drupal CMS's shim module, but was emphatically told that we cannot use adapters because if they are present in any component's input in a content template, it will crash the UI.

I can't confirm or deny that, but it was a scary enough warning that (at @effulgentsia's suggestion), I changed the approach to use a computed field instead. That has worked for us, and is implemented in the shim module.

Having said that, the true solution is probably an adapter.

🇺🇸United States phenaproxima Massachusetts

Couple minor points about comments, otherwise RTBC.

🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

🇺🇸United States phenaproxima Massachusetts
🇺🇸United States phenaproxima Massachusetts

Merged into 2.x.

Production build 0.71.5 2024