Enable more fine-grained control of enabled override user settings

Need to update test coverage still but out of time this morning, will finish later

Okay added test coverage, added update hook to maintain status quo for existing sites.

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 🌱 Add permissions for User settings Closed: won't fix

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.

Up to you of course. Looks like you closed that issue 🌱 Add permissions for User settings Closed: won't fix years ago as adding too much complexity; I tend to agree; not sure there is a case e.g. where you'd like admin role control dark mode but not let editors do that. This is much simpler.

I could create a new module, or put in a request to co-maintain that module if you aren't interested in having it in here.

For now I addressed the feedback in #6 + added further test coverage.

@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 👀

gin_permissions seems like it never had any attempt at making it work though.

Personally I think better to start simple and gin_permissions can then leverage this to control per role for someone needing that complexity. I could open an issue there like 'Future of this module' and request people comment if they do have a need for different permissions per role. We could then gauge whether there is an actual demand for that option rather than building in just in case?

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 if ultimately the two of you decide you prefer that approach, happy to take over development and maintenance of it and shift this code over to there + extend for permissions layer

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 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.

Can you have another look please? That is already the case, see https://git.drupalcode.org/project/gin/-/merge_requests/575/diffs#411dd7... + the test converage checking for 'Increase contrast' not shown, then config update only (no user data change) 'Increase contrast' being shown also validates that that is the case.

The helper.theme only addresses this comment from you earlier:

Now, if that setting gets disabled for user overrides later on, the setting will still be stored with those users.

Well sort of addresses it; it maintains the same problem as Gin theme currently has - that user data settings are only removed on resave (which is fine since the GinSettings::get() respects the enabled overrides as per above right?).

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]

Yeah if separate module is the route personally would prefer gin permissions since it doesn't really have much to do with toolbar

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.

An alternative is I reduce the scope of this MR to just refactoring the code enough to make it easy to extend.

Then e.g. with a route subscriber or something like that the gin_permissions module can be a progressive enhancement.

In which case here it can be in 'suggest' within composer.json and possibly a helptext in the current form like "Looking for more fine-grained permissions around user overrides..." type thing.

Would that work?

If so, did either of you want to be the owner of gin_permissions and make me a maintainer? Happy to make it independently too if you prefer and add you both as co-maintainers.

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

Thanks @sashaeggi

@jurgenhaas - sorry hadn't gone through the code very far, was more hypothetical, but basically with this understanding of your goals (do say if I got this wrong):

1. You want default Gin theme to be untouched (ie, you aren't happy with current merge request, e.g. comment #13 in this thread)
2. You want it to be separate permission based, and therefore in gin_permissions module instead

So in the current merge request, its not permissions based because permissions aren't available.

1. Current state: site builder can decide which options user can override. All roles get the same permissions. Example: editor, author, etc all can change only dark mode and contrast (if those two are selected as overrides).
2. State I believe you are after via Gin permissions: site builder can decide which roles have access to which permissions. Each role can have different permissions. Example: Author can change contrast only. Editor can change contrast and dark mode.

So I will work on changing to route 2 and close the current merge request I believe. Is that correct? So I think I should close this one then.

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.

Thanks for providing more detail.

Already completed

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.

Yes that's the case already with the MR + has test coverage here which I tried to explain in #13.

Here is the test coverage proving this case:
https://git.drupalcode.org/project/gin/-/merge_requests/575/diffs#586603... where:

Here is the code that ensures that only enabled settings will use the user override:
https://git.drupalcode.org/project/gin/-/merge_requests/575/diffs#411dd7...

Already completed

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.

Yes that is the case, per screenshot of the UI:

To do here

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.

Okay understood will refactor the existing code in this MR to cover this.

To do in Gin Permissions

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.

Okay created follow-up here: 📌 Create permissions per option that user can override Postponed

• Okay I refactored to add GinSettings::userOverrideAccess() and updated the code to make use of that in GinSettings::get()
• GinSettings::get() continues to be used in gin_page_attachments_alter()
• I added an alter hook in there
• I added test coverage to ensure the alter hook works

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.

Sure @jurgenhaas I have moved the contents of it over, I kept as public function, not public static function so $this can be used, figured that's better than a static callback and removing altogether, but fine to change to that if you prefer of course.

Thanks

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.

That's what I meant by #25, callback is static so removes function but then loses ability to use $this so ends up statically loading itself - matter of preference which you think is the lesser of two evils...

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.

Ooh nice TIL, will update to use that then :)

Okay that works & tests pass locally on supported Drupal BUT I attempted to update gitlab-ci.yml because its not supported by Drupal 9 (and therefore also had to update PHP version because Drupal 10.3 (lowest supported version) does not support PHP 7.4). The error in current Gitlab CI setup is "You have requested a non-existent service "callable_resolver"" because it does not exist until Drupal 10.2.

So I reverted the change and left _gin_user_form_submit in.

I think there are two routes forward:

1. Postpone then and create a separate issue to update Gitlab CI template to latest from the DA applying your eslint/stylelint
2. Resolve any issues that reveals in further follow-ups (or potentially within Gitlab CI template issue if small)
3. Re-apply the reverted change and set back to Needs Review here

Or I could create two follow-up issues to avoid this getting blocked for a longer time:

1. Update gitlab CI template
2. Convert _gin_user_form_submit to call GinSettings directly

For now started 📌 Gitlab CI template update Active as I think its first step either way, whether you decide its a hard blocker to do callable or not

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 📌 Gitlab CI template update Active now, so that we can commit them both together and all should be great.

Sure sounds good, if its okay I will wait until you have merged 📌 Gitlab CI template update Active since this will have some conflicts to resolve + can then double check that tests subsequently stay green.

Sure, let's do that.

Okay ready again, thanks for merging in the gitlab ci one!