Enable more fine-grained control of enabled override user settings

Hey @scott_euser, thanks for your contribution. I've only had a quick look into the MR and it looks really comprehensive. Just one major gap, I think hasn't been covered:

If one of the settings has been enabled and a user has altered the setting in their profile, it will be stored for them. Now, if that setting gets disabled for user overrides later on, the setting will still be stored with those users. As a result, \Drupal\gin\GinSettings::get may return an overridden value, although that's no longer allowed. That method needs to check the enabled override settings at that point.

And a minor missing tweak is in the update hook. The enabled_user_theme_settings needs to be initialized with an empty array if enabled_user_theme_settings is turned off.

I'll go for a more comprehensive review when we've sorted those out.

Thank you for your contribution here.

I have one question: Could this be solved with permissions? We've started https://git.drupalcode.org/project/gin_permissions a long time ago but it got never fully being worked on

See also #3138529: Add permissions for User settings

I'm asking as there could be a use case that you want to expose more settings to admins and less settings to content editors. But this solution seems to generally limit the settings no matter their role.

@scott_euser we closed it in favor of https://git.drupalcode.org/project/gin_permissions

So it could be worked on in gin permissions and I could add you as a maintainer if you'd like to contribute to that ☺️

But would also love to hear @jurgenhaas opinion first about a permission approach 👀

I really like the idea of making this permission based, but I don't like the idea of a separate module. I guess, it felt necessary because a them can't provide permissions? If that's the case, I'd rather put that into gin_toolbar as it is already a hard dependency for Gin and we could rely on it to be present.

But to do this step by step, I think the approach in MR!575 is a great start and could afterwards be extended with a permission controlled follow-up.

I just don't believe that the latest merge in helper.theme is appropriate. This should be handled in \Drupal\gin\GinSettings::get so that this service is the one central place which determines the correct value for each settings. Whatever feature in Gin needs to know about the setting, they should all just call that service to always receive the correct value.

I'm relying on Jürgen's expertise here, but if we can move complexity out of Gin or avoid adding it by adding it to a helper module I'm usually very happy as Gin is already quite complex on it's own. My 5 cent:

I guess, it felt necessary because a them can't provide permissions?

100%, themes can't declare permissions. So a helper module is needed

If that's the case, I'd rather put that into gin_toolbar as it is already a hard dependency for Gin and we could rely on it to be present.

That is currently the case, but once navigation is stable and takes over and we deprecate toolbar we have to evaluate the need for gin_toolbar again. So if this specific use case is handled in it's separated module it might make more sense [at least to me]

Sorry for the delay. The gin_toolbar only came to mind as it is already an existing dependency. But @saschaeggi is right, of course, if the toolbar becomes deprecated, that wouldn't be a smart move.

If we go for another helper module instead, it should be declared as a hard dependency because the code in Gin would rely on those permissions being declared.

The https://www.drupal.org/project/gin_permissions already exists, hope that @saschaeggi can maintainers there as @dermario is the owner.

Simplifying the MR sounds perfect. But I'm not sure what you mean by "progressive enhancement". So you want to alter route permissions? That wouldn't be sufficient, I guess, as we need access to the permissions when building the user profile form and when preprocessing elements, don't we?

@Jürgen @Scott I've added you both as maintainers to gin_permissions

I describe the architecture once again with my own words, maybe that makes it easier to understand:

• There should be fine-grained controls over which of the theme settings are allowed to be changed by the user and which aren't. Difference to what we have today: there is only one switch that either allows users to override theme settings or not, i.e. all or nothing.
• This part should be implemented in Gin.
• Following my comment #6 this needs some attention as there could always be a user setting around which isn't allowed to be made any longer, so then the global theme setting has to be applied.
• Getting the current values for each setting and getting the setting if a setting field is available to the current user or not, both should be implemented in a service, and all code in Gin needs to call that service to get the values and whether the user can alter the setting.
• For Gin to allow this fine-grained control, it needs extra settings where the admin can decide which settings is accessible by users. This is done without permissions because Gin should work even without the gin_permission module being installed. But those settings are only available, if the gin_permission module is not available, otherwise that module takes over.
• The second step is then to make this permission based in the gin_permission module. Here is where the service comes in, that I mentioned above: in this central place, every time Gin needs to know if the user has access to override a setting, that service calls an alter hook with which the gin_permission module can override the default Gin decision with its own permission-based algorithm.

That way, Gin can work on its own just with settings on which setting can be overridden by users. And the optional gin_permission module can hook into that decision and overrule this settings-based decision by a permission-based decision.

Re-opening the issue as it still needs some work to be done.

That looks great, thank you @scott_euser

Only one (final) consideration: should we move the _gin_user_form_submit into GinSettings as well? Then we have the settings related functionality all in one place.

Looks much better thank you. Any reason why we want to keep _gin_user_form_submit around? Couldn't the form builder refer to the submit method in the service directly? Just one fewer step that might otherwise be confusing at some point.

Ah, sorry, then I misread that.

But I guess we should be able to get both. See this change record which comes with an option "Object method, instantiated by ClassResolver, container aware using ContainerInjectionInterface", the third from the bottom of the list. That should allow us to use the method in GinSettings with a non-static method and dependency injection.

Great work @scott_euser. I'd say we go for the modern approach, so please bring back the last 3 reverted commits. There is just a tiny issue in there:

    /** @var \Drupal\Core\Utility\CallableResolver $submit_handler */

The variable should be $class_resolver instead of $submit_handler.

And then we should raise the core requirement in the info file to ^10.3 || ^11. But that can be done in the related issue. I'll review #3516730: Gitlab CI template update now, so that we can commit them both together and all should be great.

Sure, let's do that.

Excuse me @scott_euser, didn't mean to ignore this issue for such a long time. It's just that we're currently heads-down with the Gin migration into core, and we've stalled literally everything in the issue queue for the time being. Give us a bit more time until the dust settled on this major step, and we'll then get back into these topics that we'll want to get into Gin and move them forward in that new context.

I'm here to report a related issue I'm having whereby the "Admin theme settings" wrapper element gets added to every user edit form mode. I don't know if this makes sense to add here or not. This does have to do with "fine-grained control" over display admin theme user settings.

Steps to reproduce:

1. Install a standard D10/11 site with Gin Admin Theme installed and "Users can override Gin settings" option enabled.
2. Visit /admin/config/people/accounts/form-display, expand "Custom display settings" and click "Manage Form Modes".
3. Create a new User Form Mode call it anything you want, eg "Password reset" (machine name user.password_reset)
4. Return to /admin/config/people/accounts/form-display and configure the new form mode to only display a single field: "User name and password"
5. Visit /user/1/edit?display=password_reset
6. See that the "Account" fieldset is visible, and the "Admin theme settings" details element is also shown below.

Proposed resolution:

Make "Admin theme settings" available for placement via drag and drop on Manage Form Display settings on /admin/config/people/accounts/form-display

If this can be tackled independently of the awesome overhaul going on in this issue, I'm happy to split this off to a separate issue, but I don't know enough about the plans here or whether it makes sense to roll this into this feature request or not. Thanks.

In addition to #39, I found the terminology of the form elements on the user edit form a bit confusing.

Currently, the UI looks like this:

› Admin theme settings
[ ] Enable overrides
    Enables default admin theme overrides.

I’d propose the following improvements:

• Expose the field to Manage Form Display so it can be drag-n-dropped/enabled/disabled in different form modes. (comment #39)
• Update the Details element label from “Admin theme settings” to “Administrative theme preferences”.
• Revise the toggle field label from “Enable overrides” to “Personalize the administrative interface appearance”, and remove the redundant description text.
• Implement a proper Field API widget with display options similar to the Address field widget, so users can select a Wrapper type (Container, Details, Fieldset) and override the default label text for Details/Fieldset.

The benefit of this approach is flexibility: if a frontend theme also provides customization options, both admin UI and frontend UI preferences could be grouped together under one field group. For example, by setting the wrapper to Container (invisible), you could present two checkboxes side by side inside a custom field group:

› Appearance Settings (custom field group)
[ ] Personalize the administrative interface appearance.
[ ] Personalize the website appearance.

I know this is very unfortunate. There is a lot urgent staff in 5.x for Drupal 11, and there's the ongoing effort to merge Gin into core. But codebases have diverged from each other a lot. So, I'm unsure how to approach this now. Doing the work twice isn't what we want to do, I guess. For the version that goes into core, the approach would have to be revisited.

What's the status here? Should this be closed for the contributed version of Gin and moved to Gin's Core version (admin)?

That would have to happen in the main branch of core. And the php part of the theme is nothing lime it was before, everything is living in oop hooks now.

We could merge this into 5, though. But we would have to make it very clear that it won't be available afterwards. And Gin in contrib will probably not support Drupal 12.

As for starting the process for core, an issue would be the best starting point. And then getting sign off to do it before actually starting the code migration.

Haha, it's there, but it's not called Gin anymore. Go to https://git.drupalcode.org/project/drupal/-/tree/main/core/themes/defaul... and that's what used to be Gin.