Convert the Standard install profile into a set of recipes

Re #12, Test failure is due to change in recipients email in tests from admin@example.com to simpletest@example.com. How can I fix this?

Also I have added test coverage as per https://www.drupal.org/project/distributions_recipes/issues/3301370#comm...

Re: #14, I had created the empty profile to have no configuration at all. Investigating what made its way in there.

Ah, I see that functionality that cannot be currently done in recipes is being configured in the empty profile.

I had called some of that out in https://www.drupal.org/project/distributions_recipes/issues/3301370#comm...

I suggest that rather than state that only Install Profiles can do these things, that we wait until we do everything we can using recipes, and then figure out how to do the remaining items. Some of those items may bring to light additional config actions we need, or be possible with functionality on the roadmap like "default content".

I'd rather for now we leave the empty profile empty, to be used as an unadulterated clean slate for applying recipes.

Re #17, I agree with your point, but did it here in MR so that we can plan some action and then remove them. Also it seems to me that the points which you raised in https://www.drupal.org/project/distributions_recipes/issues/3301370#comm... are the only action items to make this recipe identical to standard profile.

Makes sense, thanks @narendraR!

I copied StandardTest.php code into StandardRecipeTest.php to check how much it works.
It basically failed at 2 points:

• Validating contact feedback form Recipient. We have a separate issue for this.
• Not sure why installing responsive_image and content_moderation module through service does not worked. Added @todo for this.

#20:

1. That other issue is #3303126: Getting user input during / before applying a recipe, right?
2. +1 for debugging those 2 problems here; that might lead to a new interesting insight into something Recipes is currently lacking! 😄

Exciting to see that we're already so close to have StandardRecipeTest passing! 🤩👏

1. I hope this has not been discussed previously 😅 If it has been, we should document the rationale in the issue summary.

   Why do recipe names start with "the subjective use case label" rather than "the objective thing"?

   Currently

   administrator_role
   article_comment
   article_content_type
   article_tags
   audio_media
   document_media
   

   Why not?

   comment_article
   content_type_article
   media_audio
   media_document
   role_administrator
   tags_article
   

   Is it because then it becomes harder to spot the connection between the article_* recipes? 🤔

2. Related: why even have article_comment + article_content_type + article_tags? Why not just article? Articles have comments and tags, period. If you don't want that, use a different recipe. It feels like unnecessary granularity to me.

   Heck, we could go with standard_article. And then there could be more opinionated variants of "articles" in other recipes provided by contrib?

Re #21.1, the reason is #13. I am not sure why but the value of recipients email is not changing from admin@example.com to simpletest@example.com

#23: Are you saying that we named recipes this particular way to ensure a particular execution order of the recipes? 😰

Re #24, No. I am saying this in reference to #20.

Validating contact feedback form Recipient. We have a separate issue for this.

Re #22

1.

Why do recipe names start with "the subjective use case label" rather than "the objective thing"?

As part of https://www.drupal.org/project/distributions_recipes/issues/3284777 , documentation was added to the 1.0.x branch. The naming convention is peppered throughout the documentation, but the definition of it is in this doc: https://git.drupalcode.org/project/distributions_recipes/-/blob/1.0.x/do...

I do agree with @wimleers though, I like bundle_uniquename better rather than uniquename_bundle.

Should we propose a different naming convention and update the documentation?

2.

Related: why even have article_comment + article_content_type + article_tags? Why not just article? Articles have comments and tags, period. If you don't want that, use a different recipe. It feels like unnecessary granularity to me.

Heck, we could go with standard_article. And then there could be more opinionated variants of "articles" in other recipes provided by contrib?

To make it as composable and reusable as possible.

Articles have comments and tags, period.

Not in any Drupal site I have built in 11 years. Most articles we build do not have comments, and have more specific taxonomy vocabularies like Topics, Category, Subject.

I believe this may have been a norm in Drupal 6 days of the web, and we should still support it in a composed recipe like standard_article (or article_standard).

But to make it usable, and extendable by all, it would be good to have the stack of

article_comment
article_content_type
article_tags

So I could create my own deliverable using the pieces.

Re #20,

I have found the reason for these 2 test failures.

Validating contact feedback form Recipient.

This is because standard.profile file's hook_form_FORM_ID_alter changes recipient's email.

Not sure why installing responsive_image and content_moderation module through service does not worked.

These is because tested configuration files exists in standard profile's config folder and only enabling module without standard profile will not help. Recipes do have these files and hence it worked.

Now as we know the reasons for test failures in StandardRecipeTest.php, what else can be done here.

All feedback addressed/answered, tests passing, IS updated.

Nice progress here! Some questions on the MR :)

Let's please document the rationale behind the current recipes that …/standard/recipe.yml depends upon.

That information will be crucial in eventually landing this in Drupal core. People will nitpick this apart and question why these particular recipes were created (because there is no objectively optimal or best choice). If that happens in 1 year from today, none of us will remember those choices.

Read the entire issue summary prior to re-reviewing the entire merge request. Thanks for the excellent issue summary! 😊🙏 This is definitely going to help Recipes land in core too, because we'll probably hopefully be able to copy/paste this verbatim for the future core inclusion issue 👍

Thoughts:

1. I made a few minor edits to the issue summary to improve clarity. Can you check and confirm they're correct, which would confirm my understanding? 🙏 Can you also add the sole missing follow-up? 🙏
2. Naming nit: empty is a fine name, but the inner nerd in me really wants this to be … punnier. Isn there something we can do that is borrowing cooking terminology?
3. Naming nit: I think comment_base is less precise than it could be. I think comment_structure would be better, because creating this stuff by hand would also fall under the Manage » Structure subtree of the information architecture. Alternatively, I think comment_storage also is a better name. Thinking about this more, this is actually specific to Nodes, so comment_for_nodes or comment_storage_for_node probably are better still.
4. The clear outline in the issue summary combined with the recipe-based-variant of StandardTest allowed me to not check all config in detail, but instead focus on the recipes and their dependencies. This made my life as a reviewer a lot simpler 👍
5. … which made me realize that one very valuable tool in my toolbox as a Recipe author/reviewer/auditor/evaluator would be to visualize the tree structure of all recipes 🤓🙏 Can we create a follow-up issue for that, to add it as a new console command? Tagged Needs followup for this.
6. Regarding restricted_html_format: that's the only text format available to the anonymous user. So as soon as a site decides to allow anonymous users to post comments, it becomes relevant. It's kind of like "optional config not if a module is installed but if one small tweak is made to other config". So I think it does make sense in the Standard install profile.
7. I spotted a few bugs, have a few questions, a few suggestions … but I think it's looking damn good, and will be comfortable RTBC'ing soon 😊👏

Great work here! 👍

Addressed feedback

Almost there!

Sorry, @narendraR, I got confused! I actually have zero remarks left — this is ready IMHO! 🤩

Congrats on and thank you for this excellent work. It's strong validation of the current state of Recipes! 🎉

Speaking of which: I think the next thing to do is #3400672: Robustly validate the structure of recipe.yml, while your mind still has the encountered problems fresh, and to help avoid the next recipe author from running into the same problems 😊

Updating my attribution.

Follow-up #3421660: Add a new console command to visualize the tree structure of all recipes and #3421666: Decide on naming convention on recipes created.

Should this be blocked until #3400672: Robustly validate the structure of recipe.yml is finished, to avoid committing something that might need to be changed soon afterwards?

Agreed with @phenaproxima in #49.

Who the hell self-hosts videos these days?

But I disagree with this 😅 My site has zero external dependencies (I want to own/control all content), with the exception of embedding remote videos for e.g. a DrupalCon recording on YouTube (those recordings I do not own).

@phenaproxima or @narendraR Can you please update the metadata + summary of this issue per the discussion in Drupal Slack? 🙏

Thanks!

I only reviewed this quickly, but at least on first glance, it looks great!

I'm curious why core_recommended_themes is a single recipe rather than splitting front-end theme (Olivero) and admin theme (Claro) into separate recipes. For example, might people want to reuse the Claro recipe with a different front-end theme, or reuse the Olivero recipe with Gin or some other admin theme? Is there discussion that led to combining them that I can read up on?

The work here always set both themes. Up until the following issue, it was in the primary standard recipe. I believe this is where it was broken out into its own, core_recommended_themes recipe:

https://www.drupal.org/project/distributions_recipes/issues/3301370#comm...

They could be broken into their own recipes, but for simplicity's sake, the core_recommended_themes does what it states.

There are also some sticky things with themes and recipes, like blocks are created from the frontend theme for both the frontend and admin, that we probably don't want to get into in this issue.

@effulgentsia in #54: That's a very good point! Let's do that. 💪 (@thejimbirch in #55: as long as we install the same theme, just split across two recipes instead of one, AFAICT everything should continue to work exactly the same.)

Marked Needs work for implementing @effulgentsia's suggestion.

1. currently 11/51 or 22%: #2952037: [meta] Add constraints to all simple configuration
2. currently 6/29 or 20%: #2869792: [meta] Add constraints to all config entity types

But even when we get that to 100%, that still doesn't mean all contrib config will overnight be validatable.

The good news is that I see a viable middle ground: … only validating config that is marked as FullyValidatable (i.e. has that as a top-level validation constraint).

Is that great? No.
Is that realistic? Yes.

We don’t want Recipes to become too limited to be useful by validating everything, including things that have not yet been made validatable. Because yes, e.g. type: label, type: config_dependencies have gotten validation constraints, which means that virtually all config will have some validatability. But that’s not a guarantee that all the concrete config using those types actually has been updated to comply.
Hence that FullyValidatable middle ground proposal.

If you agree, we should postpone this issue on a yet-to-be-created validation-refining Recipes issue. It can copy/paste the logic in \Drupal\KernelTests\Core\Config\ConfigEntityValidationTestBase::isFullyValidatable() to achieve what it needs.

(That would also solve the simple_config_update scenario that @phenaproxima refers to. To make that scenario (as well as any other config action) comply with this as well, we need to make ConfigActionManager::applyAction() also check whether the modified config is FullyValidatable.)

I bet that we'll run into the very same problem over at #3401925: [PP-2] After a recipe is successfully applied, validate all of the config that was imported or modified by config actions, so proposed at #3401925-8: [PP-2] After a recipe is successfully applied, validate all of the config that was imported or modified by config actions to implement #56 there. If you agree, @phenaproxima, then let's postpone this issue on that one.

My reason for postponing this in #56 which @phenaproxima agreed with in #58. I wrote in #57/at #3401925-8: [PP-2] After a recipe is successfully applied, validate all of the config that was imported or modified by config actions to implement the is FullyValidatable?-check there, but that was then extracted into its own issue: #3425540: Only validate config which is fully validatable.

#3425540: Only validate config which is fully validatable has landed, which means this is unblocked! We should not block this on #3401925: [PP-2] After a recipe is successfully applied, validate all of the config that was imported or modified by config actions, because the scope of that is bigger than the specific thing this was blocked on!

@effulgentsia's remark about core_recommended_themes in #54 has been addressed 👍

1. I think this is now truly ready, because the MR being green means more now than an February 15 in #45, since validation is much stronger now 👍
2. Thanks to #3401925: [PP-2] After a recipe is successfully applied, validate all of the config that was imported or modified by config actions having landed, we should be able to remove the "set langcode if missing" logic.
3. Since #45/Feb 15, #3418179: [meta] Make config actions more dynamic has also made big steps forward. #3420209: Allow config actions to be applied to multiple config entities using wildcards has landed, and other child issues are nearly ready. So, I re-checked this entire MR to ensure that we're using #3420209 wherever possible. But … AFAICT it's not relevant for the Standard recipe, because it defines a starting point. I think the capabilities that are being added in that meta will be more relevant for recipes like Path aliases for all content.

I do think that @alexpott would prefer to not commit this until #1170362: Install profile is disabled for lots of different reasons and core doesn't allow for that lands, but … I think we could just as easily land this now, so we can get started on the Umami install profile net, and then just remove the empty install profile that this issue is adding.

Almost ready! 😄

Done #61.2

One question on the MR, plus two requested follow-ups based on the big set of changes you've just pushed, @phenaproxima.

• Can you please create a follow-up issue to ensure that "superfluous modules installed" can be a problem we automatically detect and inform the Recipe author of? Examples: https://git.drupalcode.org/project/distributions_recipes/-/merge_request..., https://git.drupalcode.org/project/distributions_recipes/-/merge_request...
• Can you please create a follow-up issue to ensure that config.import entries are listed alphabetically and inform the Recipe author that this should be the case? Example: https://git.drupalcode.org/project/distributions_recipes/-/merge_request...

RE #65: okay, so you're saying that most of what you did in the last ~dozen commits was purely subjective preference than an objective improvement. I wish you'd described that in the commit messages and/or left a comment explaining this. I was under the impression that you deemed these changes necessary, which is why I asked for those follow-ups; to ensure that future recipes are held to the same standards (pun not intended). +1 for not creating follow-ups.

Ugh, looks like GitLab failed to post my review 😞 can't access it here because it's in the localStorage of my work laptop. I'll post it in the morning… (I can't recall if it should be RTBC-blocking or if it was minor. I bet it was minor … but I also don't want to waste Alex' time, so I'd rather err on the cautious side 😅)

All good, no worries! 't Was but a mere suggestion to allow me (or whomever is reviewing your indeed often delightful commit messages 😆) to understand your rationale and which level of rigor is appropriate :)

Posted the comment that failed to post: https://git.drupalcode.org/project/distributions_recipes/-/merge_request...

This made me wonder why some of those subjective changes are even accurate. And that is why I requested those follow-ups.

AFAICT the oversight would've been caught by #3423526: Validate that every config entity listed in the config.actions section of recipe.yml belongs to an extension that is installed, or will be by the recipe or one of its dependencies, so let's postpone on that? 😊

🤩

P.S.: there's a single line whitespace-only change in Recipe.php that could be reverted.
EDIT: the issue summary is saying that this is postponed on #3401925: [PP-2] After a recipe is successfully applied, validate all of the config that was imported or modified by config actions but AFAICT that's not actually the case?

So rather than painstakingly review all the config the standard recipes create and check against the standard profile I thought I'd make the test do that for us. This will ensure if there are change in core's Standard profile we'll find them.

This gives us some things to fix:

1) Drupal\FunctionalTests\Core\Recipe\StandardRecipeTest::testStandard
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    'create' => Array &1 ()
+    'create' => Array &1 (
+        0 => 'claro.settings'
+        1 => 'core.entity_view_mode.media.full'
+        2 => 'image.style.media_library'
+        3 => 'shortcut.set.default'
+        4 => 'system.action.media_delete_action'
+        5 => 'system.action.media_publish_action'
+        6 => 'system.action.media_save_action'
+        7 => 'system.action.media_unpublish_action'
+        8 => 'views.view.media'
+        9 => 'views.view.media_library'
+    )
     'update' => Array &2 (
         0 => 'core.extension'
+        1 => 'field.field.media.image.field_media_image'
+        2 => 'core.entity_view_display.media.remote_video.default'
+        3 => 'node.settings'
+        4 => 'core.entity_view_display.node.article.teaser'
+        5 => 'user.role.authenticated'
     )
     'delete' => Array &3 ()
     'rename' => Array &4 ()
 )

We need to fix it so that the only thing changing is core.extension. Or we agree to not do something... ie. the shortcuts. It seems we're not creating the media views. I think we should be doing that. The missing claro.settings is interesting too.

Okay we're green and found two kinda config bugs in Standard FTW. Let's do this!

Woot

Amazing! Great work everyone!

🥳🥳🥳

Best thing to happen all week!