user_user_role_insert should not exist

Re-rolled patch from #26 into a MR targeting 11.x.

Also made a second MR targeting 10.4.x

Created a draft change record: https://www.drupal.org/node/3522105 →

@joegraduate your proposed draft change record looks appropriate to me.

Is this eligible for backport to 10.x?

I updated who the CR targets and did a quick review of the 11.x MR

joegraduate → changed the visibility of the branch 2672340-remove-user_user_role_insert-10.4.x to hidden.

Created MR targeting 10.5.x.

joegraduate → changed the visibility of the branch 10.5.x to hidden.

joegraduate → changed the visibility of the branch 11.x to hidden.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

Setting back to needs review.

• MR !11992 still applies to 11.x
• MR !12041 still applies to 10.5.x

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

joegraduate → changed the visibility of the branch 2672340-remove-user_user_role_insert-10.5.x to hidden.

Hid 10.5.x MR.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. 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.

joegraduate → changed the visibility of the branch 2672340-remove-user_user_role_insert to hidden.

Created a new 11.x MR (and hid the original 11.x MR branch). Assuming that the the NR bot only checks the most recently created MR for compatibility with 11.x.

CR will need some work as believe we missed 11.2

Left some comments on the MR.

If the test changes were needed to make them pass then wouldn't this change potentially break contrib tests.

• Addressed @smustgrave's MR comments
• Added the actions as exported config yml files to config/install in the standard and demo_umami install profiles
• Added the actions as exported config yml files in the administrator_role and content_editor_role recipes

@smustgrave, yes, I think if contrib module tests involve the creation of user roles, they could be affected by these changes. I think maintainers of profiles and recipes that include custom roles are even more likely to be affected.

Then think this needs to go back to NW for backwards compatibility can’t just break things for people immediately

Think we will have to trigger some deprecation to warn people

@smustgrave, I guess I'm not sure how to best go about implementing a deprecation since the current functionality is built into the user module (in a hook_ENTITY_TYPE_insert() implementation that gets triggered any time a user role entity is created) and not part of any API code that would be invoked directly by contrib extensions.

We could add something to trigger a deprecation warning inside the hook implementation but there wouldn't really be anything contrib extension maintainers could do to address the deprecation until functionality contained in the hook implementation is removed or moved elsewhere.

I'm also not sure whether this constitutes a major change or not. @alexpott mentioned in #18 🐛 user_user_role_insert should not exist Needs work that what user_user_role_insert() currently does isn't strictly needed:

user_user_role_insert() is doing something that does not have to happen at all. It's a UI niceness but from an API point-of-view just gets in the way.

I haven't reviewed this since my earlier comment.

Hook implementations are not covered strictly under BC. I know core sometimes still provides it.

I'm not sure either how we would do it in this case.

For some ideas