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.
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:
- Creates a stable Drupal CMS project
- Adds the module/theme/recipe under test to it
- 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.
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.
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!
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.
- 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.)
- 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.)
- 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 withcore/recipes/. - 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.
- 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.
- Drupal CMS Helper is left out in the cold and doesn't get a README.md; it would be nice if it did!
.
This seems reasonable to me, with solid test coverage, but I have a few questions/suggestions.
This looks really good to me, but I suspect we can tighten scope slightly and reduce the size of the change set.
Tagging for backport to the 1.2.x branch.
I think this is a great start and a great idea. Left a few comments.
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.
Wow, this took a while but it's a nice fix. Merge train into 2.x started. Thanks!
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.
'Tis perfect. Thank you! Merged into 2.x.
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.
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.
Thanks for the patch! This was hotfixed in v1.2.8.
Reopening to create an MR for 2.0.x, which still fails the stylelint job.
I figure I'll write the tests on another MR in this issue's fork.
This should probably be backported. I suspect it will be a clean cherry-pick.
@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.
CR drafted: https://www.drupal.org/node/3552215 →
Couple of minor points but damn this looks good. We should get it committed post-haste.
I think we can make the metadata read-only and therefore remove some accessors from the event.
phenaproxima → created an issue. See original summary → .
I think the solution here is in Project Browser; blocks should not be derived based on configuration settings.
phenaproxima → made their first commit to this issue’s fork.
phenaproxima → made their first commit to this issue’s fork.
OKed by @galactus86 in person and merged into 1.x.
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.
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.
@catch, is this still something we want?
All Cypress tests were removed in favor of PHPUnit and whaddaya know...the flakiness utterly ceased.
Closing as outdated.
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.
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.
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.
Created an MR with proposed quick-fix.
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.
I was in Vienna for DrupalCon while reviewing this, so tagging accordingly.
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.
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.
@penyaskito has generously offered to handle this since I may be in the air when Canvas RC1 is released. Assigning to him.
Merge train to 2.x started. Thanks for the monumental effort here, y'all -- feels great to finally ship this.
ECA committed all blockers and released! This is no longer blocked; indeed, tests of it should be passing.
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).
Postponed on the release of Canvas 1.0.0-rc1, expected later today. Self-assigning to merge this when that drops.
Okay, responding to the points #24:
- 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.
- 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 computedurlfield as a shim -- for which we are very grateful! - 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.
- Updated as you suggested.
- Ditto.
- 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.
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.
Hmmm, tests for drupal_cms_starter seem to be legitimately failing.
@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.
Linking a well-known core bug which affects recipes that ship translation languages.
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).
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.
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?
- Spin up Drupal CMS 2.x -- no need to check out this MR branch.
- Unzip the zip file so that it is extracted at
recipes/oomami. - Go into the installer (
ddev drush sql:drop -y && ddev launch) - 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.
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.
One suggestion for clarity, RTBC otherwise.
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.
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.
Couple minor points about comments, otherwise RTBC.