Model core's standard install profile as recipes

Pushed a couple more recipes.

Started creating an ERD for what was updated. I think I may continue this as it helps my brain understand it all.

Some things that are in the Standard profiles module that adds additional functionality that may not be configuration.
* Assign user 1 the "administrator" role.
* Populate the default shortcut set.
* * Add content - internal:/node/add
* * All content - internal:/admin/content
* Sync the contact.form.feedback recipient to `site_mail`

I believe this is the big list of modules and themes that will need to verify are installed after the standard profiles are installed.

Verify these modules are installed:

- node
- history
- block
- breakpoint
- ckeditor
- config
- comment
- contextual
- contact
- menu_link_content
- datetime
- block_content
- editor
- help
- image
- menu_ui
- options
- path
- page_cache
- dynamic_page_cache
- big_pipe
- taxonomy
- dblog
- search
- shortcut
- toolbar
- field_ui
- file
- rdf
- views
- views_ui
- tour
- automated_cron
themes:
- olivero
- claro

standard.links.menu.yml

standard.front_page:
title: 'Home'
route_name: ''
menu_name: main

1. This assumes there is a main menu right? Or would this create it also?
2. I don't think we can do this in config. Content maybe?

Made some more commits this morning. Getting closer.

Need to add comments and tags into Article as that is how Standard has it.

Here is an updated (not complete) ERD.

Hi, how can I test what has been implemented in the branch 3301370-atomic-standard?
I want to move this issue forward but am unsure where to start exactly.

Hi Narendra!

The recipes that replace the Standard profile are located in /core/recipes/ on the 3301370-atomic-standard branch.

https://git.drupalcode.org/issue/distributions_recipes-3301370/-/tree/33...

The primary recipe being the /core/recipes/standard/
https://git.drupalcode.org/issue/distributions_recipes-3301370/-/tree/33...

If you look at that recipe.yml you can see the dependencies, then their dependencies...

All of the recipes could use testing of being applied, together and separate. This should be done on Drupal installed with the minimal profile.

To apply a recipe, run:
php core/scripts/drupal recipe core/recipes/standard

After each test, be sure to reset your database by reinstalling Drupal with the minimal profile.

Please let me know if you have any questions.

Thanks!

thejimbirch → changed the visibility of the branch 3301370-model-cores-standard to hidden.

ok, I couldn't get the Gitlab branch to update and it it months behind, so I made a patch. There are some differences to the MR.

1. I added a profile called Empty. Since minimal still has opinions, I tried to get Drupal to the barest minimum I could. With installing this profile, we are left with 4 modules and config that comes from system and user modules.

mysql
system
update (can be uninstalled)
user
- anonymous role
- authenticated role

2. I worked on applying a couple of the recipes, but overall, most fail, and they need a lot of work. I have learned a lot on how to crafter recipes in the last year, and some of the recipes are just plain wrong.

Here is the short list on what I worked on this evening.

standard - Segmentation fault (what is that?)
administrator_role - applies
content_editor_role - applies

3. Next steps
Test and fix every recipe starting with the smallest pieces and working up. This is going to quickly expose a few issues we know about, especially Error when installing a recipe that has configuration files already in the system. → as we have different configs that need the same modules.

4. My workflow was:

Apply the recipes patch, and the patch in this issue to a fresh Drupal installation
drush si empty to install Drupal with the new empty profile.
cd /web (if needed)
php core/scripts/drupal recipe core/recipes/{recipe_name}

Attaching patch with some progress from #26

Working article_content_type recipe patch

All recipes can be imported now. I have commented out some config actions to do so. Some recipes can be further broken down in smaller recipes. But I think now this patch require further discussion before moving further.

Nice work, @narendraR! I have added that patch to https://github.com/phenaproxima/recipe-test, which is my project to facilitate manual testing of recipes. (Feel free to clone that for your own use, and submit PRs. I can add you as a collaborator that that repo, if it will useful.)

I think a good next step here is to add test coverage that we can install the Empty profile and apply the Standard recipe. For now, we don't have to confirm that the site actually has all the stuff that Standard would provide; just that the drupal recipe command succeeds, with the Empty profile installed. Thoughts?

thejimbirch → changed the visibility of the branch 3301370-atomic-standard to hidden.

Re #30, Test added, but it is giving schema error.

Updated patch with working test.

+++ b/core/lib/Drupal/Core/Recipe/ConfigConfigurator.php
@@ -30,11 +30,22 @@ public function __construct(public readonly array $config, string $recipe_direct
+          // Remove keys with empty values.
+          foreach ($active_data as $key => $value) {
+            if (empty($value)) {
+              unset($active_data[$key]);
+            }
+          }
+          // Remove keys with empty values.
+          foreach ($recipe_config as $key => $value) {
+            if (empty($value)) {
+              unset($recipe_config[$key]);
+            }
+          }

Why was this change necessary?

This is probably not good, because won't this kick out legitimate values that are, say, empty arrays, or FALSE, or 0?

Re #35 I agree with your comment. This was done to avoid recipe fail while re-applying them due to empty `permissions` and `dependencies`.

Ah, thanks for the explanation, @narendraR. What would happen, then, if we changed that code to specifically target empty permissions and dependencies arrays?

I will update the code for only permissions and dependencies till bigger issue is solved in #3390916: Error when installing a recipe that has configuration files already in the system. → or #3283900: Define recipe runtime configuration update requirements →

First: nice work getting it this far already! 😄

RE #36 + #38

1. Why are you re-applying?
2. What exact call sequence/recipe dependency tree triggers the RecipePreExistingConfigException exception in \Drupal\Core\Recipe\ConfigConfigurator::__construct()? (Can you please add that detailed info to #3390916: Error when installing a recipe that has configuration files already in the system. → ? 🙏)
3. Can you explain what the connection is to #3283900: Define recipe runtime configuration update requirements → , because there is no update/drift in contents of depended upon recipes here? Unless that's what you mean by "re-applying": modifying the Standard recipe, re-applying it and then getting errors due to that? 🤔 As you can see: I'm clueless here, so I'd definitely appreciate it if you could explain this some more 🤓

Review

I also reviewed the recipe itself, to see if the patterns in there made sense to me. I have some questions and suggestions:

1. +++ b/core/recipes/basic_html_editor/recipe.yml
   @@ -0,0 +1,22 @@
   +  import:
   +    ckeditor5: '*'
   +    editor: '*'
   

   There's nothing to import from these, so why are these listed? 🤔

2. +++ b/core/recipes/basic_html_editor/recipe.yml
   @@ -0,0 +1,22 @@
   +#  actions:
   +#    filter.format.full_html:
   +#      setFilterConfig:
   +#        editor_file_reference:
   +#        -
   +#          id: editor_file_reference
   +#          provider: editor
   +#          status: true
   +#          weight: 11
   +#          settings: {  }
   

   Dead leftover? 😅

3. +++ b/core/recipes/basic_html_editor/recipe.yml
   --- /dev/null
   +++ b/core/recipes/basic_html_format/config/filter.format.basic_html.yml
   

   🤔 Almost nobody will want to use only the "Basic HTML" text format without the editor. I think we should change this to a single basic_html_format_editor recipe.

4. +++ b/core/recipes/standard/recipe.yml
   @@ -0,0 +1,67 @@
   +  - standard_editors
   ...
   +  - ckeditor5
   ...
   +  - editor
   

   🤔 I'd expect the Standard recipe to not explicitly list editor, filter or ckeditor5. I'd expect the recipes to handle that directly deal with those concepts (here right now: standard_editors) to install relevant modules for us.

5. +++ b/core/recipes/standard/recipe.yml
   @@ -0,0 +1,67 @@
   +  - page_cache
   +  - dynamic_page_cache
   +  - big_pipe
   

   🤔 Similarly, I'd expect these modules to not be listed but instead be a core_recommended_performance recipe.

6. +++ b/core/recipes/standard/recipe.yml
   @@ -0,0 +1,67 @@
   +  - automated_cron
   +  - announcements_feed
   

   🤔 Similarly, I'd expect these modules to not be listed but instead be a core_recommended_maintenance recipe.

Thanks Wim for review 🙏
Re 39.1: All standard profile related recipes should be able to apply individually. So suppose if someone applies tags_taxonomy first and then try to apply article_tags recipe, tags_taxonomy will be re-applied as it is part of article_tags recipe.

39.2: If we revert the changes in ConfigConfigurator.php, RecipePreExistingConfigException will trigger. One of the reason for this is because of empty dependencies and permissions config keys.

39.3: I am not sure and hence asked at https://www.drupal.org/project/distributions_recipes/issues/3390916#comm... →

Review explanation:
1. Thanks, I will remove this. I only tried to make all existing recipes run for now and will look into each recipe next.
2. I commented it intentionally to make recipe run. https://www.drupal.org/project/distributions_recipes/issues/3301370#comm... →
3. 👍
4. 👍
5. Not sure if this should be clubbed into a single recipe or not. What if someone wants to only install big_pipe?
6. Same as point 5.

So suppose if someone applies tags_taxonomy first and then try to apply article_tags recipe, tags_taxonomy will be re-applied as it is part of article_tags recipe.

This seems concerning.

Remember: as things currently stand, your site config can’t deviate from the recipe’s config, or you get PreExistingConfigException. Does this mean, effectively, that you’re locked in to the opinions of an entire recipe stack if you merely want to add some functionality?!

Re:

Does this mean, effectively, that you’re locked in to the opinions of an entire recipe stack if you merely want to add some functionality?

This is what I believe.

After applying attached patch, I did below steps:

1. Run drush si -y and drush cex -y and added config files in git.
2. Run drush si -y empty && drush cex -y && php core/scripts/drupal recipe core/recipes/standard && drush -y cex && drush uli --uri=http://recipe.test/

After doing second step, only change in configuration is related to change in uuid and removal of

-_core:
-  default_config_hash

key from files. Also I am not sure how to add override core.menu.static_menu_link_overrides.yml standard config file.

After running second step, I am getting below error:
TypeError: Drupal\Core\Entity\Sql\DefaultTableMapping::Drupal\Core\Entity\Sql\{closure}(): Argument #1 ($definition) must be of type Drupal\Core\Field\FieldStorageDefinitionInterface, __PHP_Incomplete_Class given in Drupal\Core\Entity\Sql\DefaultTableMapping::Drupal\Core\Entity\Sql\{closure}() (line 175 of /core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php). which I am not sure how to solve.

If I remove config folder from user_user_type recipe, second step will work without error and I am not getting above error.

PS: Please do not review patch(how files are added or restructuring of recipes), as I have added all configs to match it similar to standard profile and it requires lots of refactoring. But to make it similar to standard profile and find out the issues I did it in this way.

To overcome core.menu.static_menu_link_overrides.yml issue, I added it in empty profile. Still getting the same error as above. Now configurations of of standard profile and standard recipe are exactly same except default_config_hash and default_config_hash

#40

1. Could you please add a comment to the commented out pieces? That'll make this easier to understand and evolve 😊

1. … then they can create just a new recipe? Recipes are packaged opinions. Packaged recommendations. Drupal core's recommendation can be to install a bunch of performance-related modules together.
2. See above.

#41 + #42:

Re:

Does this mean, effectively, that you’re locked in to the opinions of an entire recipe stack if you merely want to add some functionality?

This is what I believe.

That's also my impression of the status quo based on what I've seen. That's why issues like #3390916: [PP-1] Error when installing a recipe that has configuration files already in the system, even if there is no difference → , #3283669: Review recipe implications for PreExistingConfigException → , #3393086: Optional configuration is imported even when module dependencies aren't there. → etc. are so important to solve prior to wide adoption.

AFAICT the reason this hasn't been considered a major problem so far is that Recipes has only been used for spinning up a new site, i.e. to reduce initial development time. The ability to apply recipes to an already existing site (and hence configured by a human) has AFAICT been considered something for a future iteration of recipes. But we are trying to change that from "distant future" to "near future" and ideally "now" 🤓

Also I am not sure how to add override core.menu.static_menu_link_overrides.yml standard config file.

The purpose of this issue is to replace the Standard install profile with a recipe. A recipe cannot contain any configuration. Therefore AFAICT you can do what you already did for core/profiles/standard/config/install/node.settings.yml and all other Standard-owned config (i.e. everything added by #2990776: Remove config-editing parts from standard_install() in favor of exported configuration → ): use config actions. An empty core.menu.static_menu_link_overrides is created upon site install, see core/config/install/core.menu.static_menu_link_overrides.yml (TIL that exists!).

After running second step, I am getting below error:
TypeError: Drupal\Core\Entity\Sql\DefaultTableMapping::Drupal\Core\Entity\Sql\{closure}(): Argument #1 ($definition) must be of type Drupal\Core\Field\FieldStorageDefinitionInterface, __PHP_Incomplete_Class given in Drupal\Core\Entity\Sql\DefaultTableMapping::Drupal\Core\Entity\Sql\{closure}() (line 175 of /core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php). which I am not sure how to solve.

Some quick searching yields https://stackoverflow.com/a/6575098 → it sounds like the autoloader does not have all the classes it should have. Either a composer install problem (i.e. it didn't create or failed to update the autoloader), or potentially a problem with a race condition between module installation and invoking logic in a freshly installed module? 🤔

If I remove config folder from user_user_type recipe, second step will work without error and I am not getting above error.

Hm that suggests my theory above is wrong. What's the full stack trace, i.e. what code is getting executed that makes it hit this point eventually?

Re #44,
I will address the review points in upcoming patches. Thanks 🙏

Above error seems to be cache issue.
drush si -y empty && drush cex -y && php core/scripts/drupal recipe core/recipes/standard && drush -y cex && <strong>drush cr</strong> && ./vendor/bin/drush uli --uri=http://recipe.test/
Running drush cr before visiting website fixed the issue. See #3315694: Cache could be cleared after a recipe installs a module →

@narendraR I updated that issue: #3315694-7: Cache could be cleared after a recipe installs a module → — can you help push that issue forward by providing exact steps to reproduce this today (it was created in late 2022, >1 year ago). 🙏

Re #46, Added steps to reproduce the issue.

Re #40, Addressed all review points.

Some refactoring is done in attached patch with all recipes working individually and standard recipe having same configuration as standard profile. This patch still needs more refactoring from my end before asking for another review.

I have made changes in recipes and it is up for review. 🙏

Nice work, @narendraR!

I added this patch to https://github.com/phenaproxima/recipe-test, then ran the custom script composer run install-from-recipe (which installs the Empty profile and applies the recipe). It worked!

Buuut...then I tried making some changes. I wanted to see what would happen if I excluded the Article content type, so I made the recipes section of web/core/recipes/standard/recipe.yml look like this:

  - basic_block_type
#  - article_comment
#  - article_tags
  - feedback_contact_form
  - page_content_type
#  - standard_content_types
  - standard_editors
  - standard_roles
  - user_user_type
  - restricted_html_format
  - core_recommended_performance
  - core_recommended_maintenance
  - core_recommended_themes

(Removed the Article stuff and standard_content_type; added page_content_type.)

And...got an error on my next run of composer run install-from-recipe:

In RecipeConfigInstaller.php line 53:
                                                                   
  The configuration 'views.view.frontpage' has unmet dependencies  

When I added a debug line, I discovered that the missing dependency is core.entity_view_mode.node.rss.

This points to some sort of bug, though I don't have time to dive into it. Why does removing the Article content type cause the RSS view mode to stop existing?

Ah, I see why the problem in #49 happens -- it's because only the article_content_type recipe actually bothers to import that view mode.

I believe dependent recipes are applied first.

While the standard/recipe.yml requires core's node module's config you need, the page_content_type/recipe.yml does not.

#50 seems to imply that to achieve truly reliable and composable recipes, that we should have automatic test coverage for each recipe that verifies its installability.

#51 points out there is one missing dependency. Right now finding the culprit is a manual process (which @phenaproxima did in #50).

I propose that we do NOT fix this manually, and instead first write a test that is able to test all discovered recipes, and verifies they're all complete wrt config dependencies. That should result in @phenaproxima's finding being discovered automatically.

In other words: let's automate this once now to allow iterating faster on this patch! (Because the composition of each recipe is bound to change before this gets committed, and every such refactoring is likely to introduce new bugs due to oversights — which is normal for humans, so let's let the computers do it for us 🤓.)

I kinda tend to agree with Wim here. Standard's dependency tree is very dense, and we will almost certainly miss things if we try to keep it in our brains.

Would this be a test that is part of the Standard Profile, or of Recipes? If it the later, shouldn't that be another issue?

I don't do much with core tests. Would this be something a recipe creator could run on their local to validate their recipe dependencies?

#53: 👍 Adding one more tag: once the test exists and has proven to be helpful, we'll want to extract that test coverage into a separate issue, like @thejimbirch suggests in #54.

#54: The latter. But it should be developed here first, because this is by far the most complex set of recipes. Once the test has proven to be working (i.e. automatically identify what @phenaproxima identified in #50), then we should convert it to a separate issue + MR with explicit test coverage. That can then land first, and then this MR can be rebased (and be smaller again).

Would this be something a recipe creator could run on their local to validate their recipe dependencies?

Eventually, yes. It's related to #3400672: Robustly validate the structure of recipe.yml → . #3400672 currently is scoped to only verify that the structure of a recipe.yml file is complete (all necessary key-value pairs defined) and consistent (for example: if a recipe installs module FOO, then it must also specify which config entities of FOO to install). What's being described here (#52) is going further: if we're talking about a BAR recipe that depends on FOO and BAZ, then it'd verify that for all config entities installed/modified by BAR, that their config dependencies are met thanks to FOO and BAZ.

Okay, we don't know how the hell this happened but the issue fork is all bungled up - possibly because it was opened before 11.x was branched.

At @alexpott's suggestion, I'm closing this issue out and we'll move over to a new issue -- #3417835: Convert the Standard install profile into a set of recipes → -- and a new issue fork therein. All credit has been transferred.

See you there!