Config whitelist to allow "some" configuration to be edited while lock is active

Maybe this is related to your proposal.

I would like to unlock the /admin/structure/menu/manage/main form to allow users to drag menu items around and change the order.
I wonder if the weight of a menu link is content not config.

I am interested in your patch! I had to disable this module.

I think this makes sense from a feature point of view. I didnt code review, as patch looks a bit large.

Calling it "Exceptions" is very confusing. It's a whitelist as asked for here: #2851286: Consider a whitelist to allow "some" configuration to be edited We need to rename all the things in the patch.

This patch is also too big and complex. I could possibly support a white list of config names in a simple config object. Having a service is confusing and overkill I think.

The whitelist config object should never be able to be modified in production.

1. +++ b/src/Config/ConfigReadonlyStorage.php
   @@ -98,11 +100,17 @@ class ConfigReadonlyStorage extends CachedStorage {
   +   * CHeck whether config is currently locked.
   

   Comment starts with uppercase C and H.

2. +++ b/src/ConfigReadonlyWhitelistTrait.php
   @@ -0,0 +1,127 @@
   +   * Get exceptions from config whitelist file.
   

   Are we happy to leave "exceptions" in here?

3. +++ b/src/ConfigReadonlyWhitelistTrait.php
   @@ -0,0 +1,127 @@
   +   * Get single pattern from an exception.
   

   Same as above.

I could not see how to get the test to test for instance node type page or article as I get a page not found at admin/structure/types/manage/article for instance even with adding node as a dependency of the test.

Node module doesn't create any content type, the "standard" profile does. You'll need to use drupalCreateContentType(). I get some fatal errors adding this myself to the test, but probably it's just my instance.

After further tests:

1. +++ b/src/Config/ConfigReadonlyStorage.php
   @@ -98,11 +100,17 @@ class ConfigReadonlyStorage extends CachedStorage {
   +    $this->checkLock($name);
   

   $name variable doesn't exist in this context.

2. +++ b/src/Exception/ConfigReadonlyWhitelistException.php
   @@ -0,0 +1,8 @@
   +namespace Drupal\Core\Entity\Exception;
   

   Name space should be Drupal\config_readonly\Exception

3. +++ b/src/ConfigReadonlyWhitelistTrait.php
   @@ -0,0 +1,127 @@
   +      throw new ConfigReadonlyWhitelistException($error);
   

   ConfigReadonlyWhitelistException class can't be found without the use statement.

Apparently you can't use $this->writeSettings() twice in the same test, even with $this->resetAll(). Not sure if this is expect or a bug in core test framework.
Splitting in 2 tests works.

Besides I would personally separate the WhiteList tests in a separated class. In this way the 'node' dependency is not loaded when not needed (i.e for original config_readonly tests). But that is probably me being too picky.

ConfigReadonlyWhitelistTrait doesn't take in consideration the case when the setting "config_readonly_whitelist" is there but no file is provided ($whitelist is an empty string) and the case when the file is empty (returned $pattern is undefined).

This is an interesting and useful feature, really looking forward for using it!

Test fail due modules form being changed. I can't find exactly the commit, but I can see from the interface modules[Core][action][enable] should be now modules[action][enable]. That should probably be fixed before any other progress can be made here.

I've created #2851929: ReadOnlyConfigTest fails when using core 8.3.x or 8.4.x branches and submitted a patch. Until that is merged in we can temporary test against 8.2.x, as that should pass.

Patch at #27 looks good to me, if test is clean I'm assuming this can be RTBC.

I'm not sure I understand the motivation of using an external file as a whitelist source.

I think the whitelist itself should be a config object that cannot be changed while config is locked. Then you could track that together with other config changes?

Hello, please allow me to chime in.

Adding a text file is in my opinion not a good strategy.
configuring it via a configuration object can make sense, but to interact with it from another module one would have to start overriding configuration just for that.

What I had in mind was a service collector or a plugin, or at the very least a hook.
A hook seems to me to be the simplest form for now as you can always create a small module that implements that hook and reads from a text file, or from configuration.

Allow me to explain where this comes from: I maintain config_split, which is in my humble opinion a very elegant solution to prevent configuration of clients to be overwritten and to maintain development modules and config only in that environment. I am planning also to make it extensible so that other modules such as config_ignore could benefit from the same approach. It works with the basic drush commands as well (or it will as soon as I apply the patch in #2839452)
My plan is to make config_split and possibly config_ignore work with config_readonly.

PS: I also incorporated 2851929-2 and I added the whitelist for simple configuration forms.

What version of Drupal & Config Readonly are you doing this with, Scott? I'm still having a hard time getting the patch to apply.

I wanted to allow users to reorder menu links (/admin/structure/menu/manage/main). I applied the patch and tried to whitelist 'system.menu.*' in the hook but it did not work.
After looking at the code, I figured out that the form is a config entity form, so I have to put the name of the entity in the whitelist: 'main'. But it seems to me like it's too wide and could lead to false positives because there is no "namespace". Some other config entity could also be named 'main', no?

Actually, replacing $name = $entity->getConfigTarget(); with $name = $entity->getConfigDependencyName(); in ReadOnlyFormSubscriber.php (at "// Don't block particular patterns.") solves the above problem. I can use system.menu.* in the whitelist.

But: I still have an error after reordering menu links when saving my view, because some menu links are config and not content (for example, a page view whose menu link is configured in the view configuration page). So I would have to whitelist all of these links, which implies allowing too much things (in the previous example, it would allow to edit the view although config readonly is activated).
I'm wondering if the problem of menu items reordering can be handled with this whitelist system. This might need another issue queue.

Edit: and here's the issue queue: #2892631: Don't block content menu links reordering

Here's an updated patch to fix #49 by doing what I said in #50.

Thanks for your efforts finding a solution to whitelisting certai configurations. What's not clear for me:
With the latest patch applied against dev – how would I define my whitelist?

This is a really good solution. I think will need a future revisions, as per #37 and Config Filter, but this is more a overall Config Readonly roadmap and so to be discussed with maintainer(s) and generally out of the scope for this issue.

I've also updated the IS to reflect latest changes, as current version still mentions the yaml text file in the codebase for whitelisting.

I have created Config Readonly Filter to solve this very same problem. As these configurations could be site/environment specific, having them as config entity and export with default configuration would be better. Probably we could add alter/hook to the discovery as well in future.

@vijaycs85 I think that this filter module should be in part of the main module.
What do you think? Is possible to merge it?

@landsman sure, if we could add.

New patch attached:

• I think we need an easy settings.php solution so I added config_readonly_readonly_whitelist_patterns to add patterns found in settings.
• Any comment that says "this is a dirty hack. Look away" is going to get looked at closely. I've reworked that code to just document what it's doing.

I've tried to do minimal work on this patch. I did not address the following in my new patch:

• I would prefer a service than a trait, given that $moduleHandler is defined in ConfigReadonlyWhitelistTrait but set in whatever uses the trait, and so that there's only one static cache of the whitelist (I am not sure, but a think use of the trait will have it's own local variables). I disagree that the service added a lot.
• I'd like to see https://www.drupal.org/project/config_readonly_filter as a submodule or built into this patch more directly, so that the menu reorder use case is solved directly here.

@vijaycs85's solution is just another solution. I have not reviewed it. But as another contrib module solution, it shouldn't derail the work here.

Marking back to needs review for my most recent changes.

Rerolled #59 which would not apply, by applying #51 and then the interdiff #59 on top of it.

I had a look at the changes, my comments:
1. I think adding the settings.php option is a good addition as it opens up the possibility of defining these settings per environment.
2. The refactor of the "hack" is a great idea.

I tested the following:

With this on my settings.php:

$settings['config_readonly'] = TRUE;
$settings['config_readonly_whitelist_patterns'] = [
  'system.menu.*',
  'core.menu.static_menu_link_overrides',
];

I was able to edit any menu on the system, tested:

1. Changing the Title of the menu.
2. Changing the Description of the menu.
3. Adding new menu items to it.
4. Deleting menu items from it.
5. Enabling and disabling menu items.
6. Re-ordering menu items.
7. Editing menu items.
8. Reseting menu items.

This all worked flawlessly, so switching back to RTBC.

in general, someone else should review if you wrote the last patch

A few nits before this is done.

A bunch of @throws ConfigReadonlyStorageException should be fixed to use the full class name of the exception.

A place or two missing @throws, and some other doc comments.

1. +++ b/src/Config/ConfigReadonlyStorage.php
   @@ -58,51 +65,59 @@ class ConfigReadonlyStorage extends CachedStorage {
   +   * @throws ConfigReadonlyStorageException
   

   Usually use fully namespaced name here

2. +++ b/src/Config/ConfigReadonlyStorage.php
   @@ -58,51 +65,59 @@ class ConfigReadonlyStorage extends CachedStorage {
   +   *   Check for a specific lock name.
   

   This is a config name, not a lock name?

3. +++ b/src/Config/ConfigReadonlyStorage.php
   @@ -58,51 +65,59 @@ class ConfigReadonlyStorage extends CachedStorage {
   +  protected function checkLock($name = '') {
   

   does this needs an @throws?

The new exception:
class ConfigReadonlyStorageException extends \Exception {}

should perhaps extend something a little more specific like \RuntimeException, and have a \n in the braces like core?

class AccessException extends \RuntimeException {
}

Addressing #64 - thanks @pwolanin for the review

A few more minor comments. (Sorry I didn't notice these before. The first one is the only really important one.)

1. +++ b/src/EventSubscriber/ReadOnlyFormSubscriber.php
   @@ -1,23 +1,31 @@
   +  /**
   

   Add a local variable for protected $moduleHandler.

2. +++ b/src/EventSubscriber/ReadOnlyFormSubscriber.php
   @@ -41,20 +49,39 @@ class ReadOnlyFormSubscriber implements EventSubscriberInterface {
   +    if ($mark_form_read_only && $form_object instanceof EntityFormInterface) {
   

   I'd make this an elseif instead of an if. It is mutually exclusive from the previous conditional.

Addressing #66.

Thanks @manuel-garcia!

@manuel-garcia, this is confusing that $moduleHandler is declared in the trait, but set in the class. Can we refactor so that it is defined and set in the same place? I suggest that we add a setter function to the trait, and call the trait setter function from the class constructor. That way the variable that the trait uses is self contained in the trait.

The change to an elseif suggested in #66.2 is invalid. Reason being that the if the first if block runs, then $mark_form_read_only is assigned again.

Re #72 I think that would be a strange pattern to implement. On the latest patch the module handler is not declared on the trait although it makes use of it on getWhitelistPatterns().
$moduleHandler is declared on the two classes that do use the trait (ReadOnlyFormSubscriber and ConfigReadonlyStorage).

I also tried a couple of other things:

1. Pass the module handler to the trait methods. Still a strange thing to do, so no patch for this.
2. Get rid of ConfigReadonlyWhitelistTrait::getWhitelistPatterns, and have the trait just hold matchesWhitelistPattern accepting a name and array of patterns. To me this seems like the cleaner trait, yet it means we having to get the patterns on several places instead of one.

So I am leaving the patch as is right now, perhaps the maintainers can weight in on this, I attach the interdiff of what implementing #72 would look like just in case.

Looks at examples in core some do seem to have setters or methods to interact with the variables defined by the trait.

see e.g. \Drupal\Core\Entity\DependencyTrait

in other cases they access the trait variable directly like

\Drupal\Core\Database\Query\InsertTrait

The variable in this patch is a service, which isn't something I've found used yet in core. I think I'd lean towards defining the property in the trait since it's used there. Or possibly requiring it to be passed into the trait;s method calls, but that seems like unneeded complexity.

A question, why is this method even public? seems to be used only internally to the service?

public function matchesWhitelistPattern($name)

Thanks @pwolanin for weighting in on this, adding a setter for the module handler to the trait here.

Oops.

Re #75 I couldnt find a reason not to, so declaring matchesWhitelistPattern as protected here.

I just ran into an error when deploying one of our sites with this patch. I'm not sure yet the cause of the error, so I'm removing RTBC until I track it down. It might just be that we need to do a cache_clear before this runs, because of the new services, but let me track it down before anyone commits it.

Marking this as RTBC now, we figured out that our builds need an additional 'drush cr' in our deploy scripts.

The use of the reflection class is a bit icky - is #2095289: Make getEditableConfigNames() public and use it in config_translation stalled? Looks like alexpott was willing to consider 8.x solutions.

Still a bit unsure why setModuleHandler() and getWhitelistPatterns() should be public (api additions)

@pwolanin Yes the reflection class is icky. We have no other possible solution in 8.x. Changing it is an API break. I tried to document this in the code as cleanly as possible by giving it it's own method, and by adding a @todo that says this can't be fixed in 8.x.

Marking back to RTBC after testing @manuel-appnovation's changes. We still have the reflection. The best we can do is document this. I think it's documented sufficiently in the code, and that documentation has been reviewed here by a couple people. It's up to @pwolanin to accept or suggest how better to document it.

Hi,

This is amazing module. Thanks everyone for the contribution. I would like to allow editing to admin --> people --> Permission and also for roles.Currently this modules locks down all the pages in configuration and also core modules - like installing modules.

For those looking for documentation on this new feature it is here https://cgit.drupalcode.org/config_readonly/tree/README.txt