Roles should be in their own recipes for composability

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

I was initially unsure about this, but after some discussion in Slack with @thejimbirch, I'm convinced.

For me, this completely clarifies the use case for ensure_exists -- "if this thing already exists, go ahead and use whatever we've got; otherwise, create it with these values". That makes perfect sense for these two roles, which I can absolutely see being useful on their own in various recipes. I imagine almost every site would want at least one administrative role, and a content editor also seems like a useful role to have, but not necessarily both. So although they are certainly minimalist, and of dubious value in and of themselves, they make sense as individual building blocks.

So I'm +1 for this change, and I think it will make other recipes easier to write. The code looks good to me and passes tests, and we already have coverage to prove that all core recipes can be applied on their own.

Possibly this issue can be used as an example to better describe why / when to use ensure_exist config action https://git.drupalcode.org/project/distributions_recipes/-/blob/1.0.x/do...

Couple of questions on the MR about why the different permissions live in their respective places.

Failing tests :(

Addressed test failures & pipeline passed successfully.

Please review, moved NR

Thanks for fixing the test! Unfortunately, it's probably not the proper fix for this situation. It's not obvious from the code, but those assertions were added in order to confirm that the Standard recipe suite generates config identical to the Standard profile. So we actually need to go back and find out why the roles differ, and fix that, rather than change the test itself.

Figured it out; the problem is that, in the Standard profile, the content_editor role doesn't get the "access content" permission. That's granted to anonymous and authenticated users. I adjusted the recipes to match and the test passes for me locally now.

Observed test failures, found there are some PHPUnit test case failures unrelated to MR & run pipeline

pipeline passed & MR is mergeable now.

I have applied the patch from gitlab using

composer config --json --merge extra.patches.drupal/core '{"#3454605: Roles should be in their own recipes for composability": "https://git.drupalcode.org/project/drupal/-/merge_requests/8413.diff"}'

I can see that the patch applies cleanly and makes the changes described.

I have also attempted to run:
php -d memory_limit=512M core/scripts/drupal install core/recipes/standard

Which completes as expected.

Crediting @the_g_bomb for manual testing!

Code looks good, Tested locally and confirmed both recipes are independently composable. RTBC

Crediting @b_sharpe for review/testing.

Pulling default config out of Standard and into modules is a totally sensible and allowable minor-only change (since it only affects things at install time, not at runtime). This seems like the equivalent of that, so +1.

To review this, I actually put two copies of the MR side-by-side to make sure all the permissions were the same and whatnot. I see that the most basic system permissions are assigned to the roles themselves, and then anything associated with various uninstallable modules is still part of the Standard recipe. Makes sense, and seems correct especially after reading the above discussion.

Committed to 11.x. As a minor-only kind of change, the target version for this is probably 11.1. However, I notice the issue was filed against 11.0.x; was that intentional or just an accident because the issue selector sorts 11.x below it?

11.0.x is in beta and 10.3.x is in RC and days from release. 10.4.x is a maintenance minor, so only gets minimal things like dependency updates, security fixes, and APIs additions related to deprecations that make it impossible for contrib to support D10 and D11 simultaneously. However, recipes has kind of a weird status, so I am going to discuss backportability with the other release managers.

Would it negatively affect Starshot if this were not backported? Keeping in mind that installer changes and the like for Starshot wouldn't exist before 11.1.0 anyway.

Setting PTBP and tagging for RM review to discuss the backport thing.

Thanks everyone!

Would it negatively affect Starshot if this were not backported?

Speaking only for the prototype: Starshot has worked around, and probably could continue to work around, the fact that the roles aren't (yet) composable in 10.x, but it would be better if they were. So I'm personally hoping this does get backported.

Discussed with @catch. Given the fact that this is a fairly harmless disruption to an API/config that has "internal/experimentalish" status, and given @phenaproxima's statement about the value to the initiative, we'll backport this all the way to 10.3. If there were more API surface or more disruption, or if it were less important, it would be 11.1 only. Thanks!

Done. Thanks everyone!

Bot ate my commit credit.