I'm not one to lightly use the Critical status, but:

  • Drupal 9.3.0 is due to be released in 2 days
  • It contains new code that breaks Group's permission UI

Without the permission UI, the Group module is basically useless. See the following bug report in Group #3250536: Add support for the manage permissions tab for bundles coming in 9.3.0 which was caused by core issue #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions

Can we please rethink the path used in the core issue? That would be an easy short-term fix to avoid Drupal 9.3.0 breaking everyone's Group install as I'm currently stretched for time to chase this core release with new Group releases.

Steps to reproduce

  1. Install the Group module
  2. Go to /admin/group/types/add and add a group type
  3. Notice that all admin actions for that bundle are at /admin/group/types/manage/{group_type}/FOO
  4. Try to edit the group type's group permissions at /admin/group/types/manage/{group_type}/permissions
  5. See the new core screen instead

This is because Group could have never anticipated there being a "permissions" path added to the bundle space and therefore used said path as it was redundant to call the final path part group-permissions, given how all group admin routes start with /admin/group.

FYI, Group has even more of these admin paths, such as /admin/group/types/manage/{group_type}/content that shows all of the installed content plugins. If ever core were to add a tab to all bundle admin screens with a view of content using said bundle, we'd be in the same tough spot. But that scenario is possibly even more unlikely than this one, which turned out to be not so unlikely after all :D

Comments

kristiaanvandeneynde created an issue. See original summary.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Issue summary: View changes
kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented
Issue summary: View changes

Adding steps to reproduce.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#3250536: Add support for the manage permissions tab for bundles coming in 9.3.0
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#2934995: Add a "Manage permissions" tab for each bundle that has associated permissions
cilefen’s picture

cilefen commented

@alexpott Are you going to revert? I don’t see this being adjusted in time.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@cilefen I'm discussing it will @catch.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

I don't think a revert is called for, perhaps just change the path to something else?

The bigger issue is obviously the uncertainty surrounding the "bundle path namespace" as we have the tabs that third party modules add (using the module name as path part) and those that core has added with Field UI for years now. Outright forbidding core to add any new tabs to the bundle UI seems a bit harsh. Maybe compromise when they do collide is good enough, in this case why not call it /admin/group/types/manage/{group_type}/bundle-permissions or something?

catch’s picture

catch
he/him
Primary language English
commented

I personally think the path that was picked in #2934995: Add a "Manage permissions" tab for each bundle that has associated permissions is pretty good, and it'd be better if the group permissions path was namespaced (like group-permissions). However since this has caused a blocker for groups, I've reverted it from the 9.3.x branch only for now.

If we don't do anything else, groups will need to update the path name in time for the 9.4.0 release. Or we could recommit to 9.3.x with a different path name, or something else we haven't thought of yet.

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

Thanks!

As I explained to Alex on Slack, the reason group freely uses "permissions", "roles", etc is because it is namespaced in /admin/group and the group types' individual sections. I thought it was reasonable to assume I would have near full control over my entities' UIs.

It also makes paths like the below possible, rather than their ugly counterpart:

  • admin/group/types/manage/MY_TYPE/roles/MY_ROLE/permissions
  • admin/group/group_types/manage/MY_TYPE/group_roles/MY_ROLE/group_permissions

You don't see Commerce prefixing every label with Commerce in the Commerce admin UI either.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

It feels like Group should add to (or replace) the new permissions page, rather than having two separate "permissions" tabs on each bundle; surely users are going to be confused between "bundle permissions" and "group permissions"?

Maybe the core route subscribers also need to be a bit more lenient - should they be weighted quite low and abort if they find a route already exists at a path that they want? This would let other modules override them easily.

Also, should the route compiler complain if two routes want to use the same path?

kristiaanvandeneynde’s picture

kristiaanvandeneynde
Primary language Dutch
Location Antwerp, Belgium
commented

The two pages are vastly different @longwave. One deals with group permissions for that group type, one deals with core permissions.

The funny part is that group types only have one bundles-specific permission: "My bundle: Create new group". I would really not like to have a page for a single permission, but right now I need to write code to opt out of the new core page. I'd rather have it opt in instead.

I can imagine other modules not having a need for the screen either because they don't have any or very few bundle permissions, or because they have not implemented the CR yet that allows them to let permissions define what bundle they're for.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

I can imagine other modules not having a need for the screen either because they don't have any or very few bundle permissions, or because they have not implemented the CR yet that allows them to let permissions define what bundle they're for.

This is completely true - at the moment the bundle permissions screen for the group entity is completely empty - this is because the dependencies are currently not being added. The ability to do this was added in 9.3.x too. I was thinking we could tie the new bundle permissions page to the entity being a EditorialContentEntityBase since the reason we have so many bundle level permissions is because of the complex editorial behaviour but groups now implement that to.

I think that opt-in via something on the entity type annotation is going to be the easiest path forward here.

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented

#3253955: Let modules opt in to the bundle-specific permissions form is in RTBC status

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

cilefen’s picture

cilefen commented

Is this still an issue now that #3253955: Let modules opt in to the bundle-specific permissions form landed?

catch’s picture

catch
he/him
Primary language English
commented
Status: Active » Closed (duplicate)

This should be fine now with the changes over there. Going to mark as duplicate.