Add ConfigSchemaChecker to development.services.yml

Nice idea!

Here is a patch for this issue.

Please add a todo comment in the `ConfigSchemaChecker` class suggesting it to be moved out of `Testing` namespace in D9 so that we don't forget to do so.

Added the todo in the ConfigSchemaChecker class.

LGTM

1. +++ b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php
   @@ -5,6 +5,7 @@
   +// @todo Move out of Testing namespace in Drupal 9.
   

   @todo's without issues aren't correct. Plus we could copy the class to the "correct" place now and just make the existing on extend from that.

2. +++ b/sites/development.services.yml
   @@ -5,3 +5,5 @@
   +  config.checker.schema:
   +    class: Drupal\Core\Config\Testing\ConfigSchemaChecker
   

   I think this needs to be be tagged with...

       tags:
         - { name: event_subscriber }
   

   Otherwise it is not going to work.

@alexpott:

• About namespace, do you see SchemaChecker being used anywhere other than during development? If so, should we use "Devel" or "Development" as the namespace? That way, we could add other devel only config classes into this namespace
• As for the tag, we need name: event_subscriber because the ConfigSchemaChecker implement EventSubscriberInterface, correct? So, we just need to add the line you'd mentioned?

We can create a patch once we have the namespace in place.

• Patch contains namespace/directory as ./Drupal/Core/Config/Schema/ConfigSchemaChecker.
• Added the "tags: ... to development.services.yml"

Think this patch is invalid and here more for reference -- don't think the directory rename is in the patch. Did a 'git diff -M10%' but that doesn't seem to have worked; putting this anyways so someone else (or a clearer me) can clean up later.

+++ b/core/lib/Drupal/Core/Config/Schema/ConfigSchema/ConfigSchemaChecker.php
@@ -2,10 +2,10 @@
-namespace Drupal\Core\Config\Testing;
+namespace Drupal\Core\Config\ConfigSchemaChecker;

You cannot change the namespace without also moving the class to the new directory, as in, on the file system.

Does someone mind explaining how this actually works at the moment? How can Drupal find this class?

Thanks to cilefen; have the directory move as part of the patch (Hopefully)

Someone please update the issue summary when the namespace decision is made. This is going with "Devel" for the namespace.

+++ b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php
@@ -25,87 +16,9 @@
+ * @deprecated in Drupal 8.1.0 and will be removed before Drupal 9.0.0. Use
+ *   Instead use Drupal\Core\Config\Devel\ConfigSchemaChecker.

Oops, I messed up the first line here.

Changed it. Please review.

@tstoeckler I do not understand your namespace suggestion in #11 and I regret ignoring it rather than just saying so.

Just a nitpick, let's use 'Development' instead of 'Devel', we don't use abbrev. in our variables, so ideally also not in namespaces.

Patch no longer applies.

Updated issue summary to indicate that a reroll is needed and to indicate that the namespace needs to be agreed on. At least, it wasn't clear to me that there is agreement on using the 'Development' namespace.

Leaving as novice for the reroll.

reroll of #27. There was only one little conflict at this file /core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php

Just some nitpicks. I think this looks overall great.

1. +++ b/core/lib/Drupal/Core/Config/Development/ConfigSchemaChecker.php
   @@ -0,0 +1,111 @@
   +/**
   + * @file
   + * Contains \Drupal\Core\Config\Development\ConfigSchemaChecker.
   + */
   

   This is no longer needed

2. +++ b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php
   @@ -19,88 +12,10 @@
   + * @deprecated in Drupal 8.1.0 and will be removed before Drupal 9.0.0.
   + *   Use Drupal\Core\Config\Development\ConfigSchemaChecker.
   

   Let's update that to 8.3.0

I think this at least major for the DX. Agree this looks great!

Both nitpicks can be fixed on commit.

1. This is a really nice idea
2. The patch should be created with the following gitconfig...
   

   [diff]
   	renames = copies
   

   So it is eay to tell no unnecessary changes have occurred in the new class. See https://www.drupal.org/documentation/git/configure

3. +++ b/core/modules/simpletest/src/KernelTestBase.php
   @@ -321,7 +321,7 @@ public function containerBuild(ContainerBuilder $container) {
   -        ->register('simpletest.config_schema_checker', 'Drupal\Core\Config\Testing\ConfigSchemaChecker')
   +        ->register('simpletest.config_schema_checker', 'Drupal\Core\Config\Development\ConfigSchemaChecker')
   
   +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -608,7 +608,7 @@ public function register(ContainerBuilder $container) {
   -        ->register('simpletest.config_schema_checker', 'Drupal\Core\Config\Testing\ConfigSchemaChecker')
   +        ->register('simpletest.config_schema_checker', 'Drupal\Core\Config\Development\ConfigSchemaChecker')
   

   ConfigSchemaChecker::class and a use statement would be preferable.

4. Let's address #32 too

Creating the patch with changes in #32 and #35
(including gitconfig:

[diff]
	renames = copies

)

1. --- b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php
   +++ b/core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php
   

   This file is no longer being moved.

2. +++ b/core/modules/simpletest/src/KernelTestBase.php
   @@ -321,7 +322,7 @@
   +        ->register('simpletest.config_schema_checker', 'ConfigSchemaChecker')
   
   +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -608,7 +609,7 @@
   +        ->register('simpletest.config_schema_checker', 'ConfigSchemaChecker')
   

   ConfigSchemaChecker::class

   The current code will fail.

Sorry @Wim Leers, I should remove the `core/lib/Drupal/Core/Config/Testing/ConfigSchemaChecker.php` file?

Fixing Patch

Looks perfect :)

+++ b/core/modules/simpletest/src/WebTestBase.php
@@ -547,7 +547,7 @@ protected function prepareSettings() {
-        'class' => 'Drupal\Core\Config\Testing\ConfigSchemaChecker',
+        'class' => 'Drupal\Core\Config\Development\ConfigSchemaChecker',

I missed one... sorry...

Also as we're deprecating something we should have a CR to tell developers about this.

@alexpott on BrowserTestBase is also call for use Drupal\Core\Config\Testing\ConfigSchemaChecker. Should I change here to use the Drupal\Core\Config\Development\ConfigSchemaChecker, right?

updating patch

Fixing patch #46

Yep we need to fix the usage in \Drupal\Tests\BrowserTestBase - still not fixed in #48 - well spotted to @faline. And there is an @see to fix in there too.

Sorry @alexpott I forgot to include the change on BrowserTestBase.
Now the fixed patch included the @see fiz too.

@tstoeckler I keep both @see references, old and new one, what do you think?

@tstoeckler I forgot to change this point: @see on \Drupal\Tests\BrowserTestBase as @alexpott told on comment #49

Testbot failure, resetting status due to https://www.drupal.org/node/2828045

Committed/pushed to 8.3.x, thanks!

On top of that I've also people struggling to use basically anything in the views UI due to issues like #2623568: Config schema of argument_default plugins is incorrect and some others. Of course they could change their development.services.yml, but that's not obvious.

Given that we should probably roll it back and figure out how to at least solve the update problem, as described by @tstoeckler

Committed #64 to roll this back.

What if the checker will emit an error instead of throwing an exception? We could make the behavior configurable to preserve exceptions for testing. This checker is really needed for local development.

Meanwhile I just created a simple module to test how it could work.

This is at NW if it's still something being pursued at all.

We are using config schema checker as a default dev requirement in our all our projects and possibly there is a similar issue with it that was mentioned in #65. I do not have full details yet, but it seems when contrib modules changes their config schema in a release and also remove their old config definitions (afaik there is no policy for not doing that in a non-major version) then the update process (updb && cim) fails false positively with config scheme validation error.

Eg.: https://www.drupal.org/project/eu_cookie_compliance/issues/3210365#comme...

Had a crosspost.

> fails false positively with config scheme validation error.

That's not a false positive, that's the expected behavior then. Removing the schema means it's no longer valid. Yes, not, running it during updates might make sense, but even then, just removing the schema is tricky.

Drupal 9 now has a way to deprecate config schema, but that's AFAIK quite new and contrib won't have picked that up yet. But that's probably the way forward for most cases.

What if we add the schema validation as a warning when one saves the config. (ie idea from #74)

I understand that we can not just throw an exception on developers machines, especially given the state of config schema in contrib (and probably even worse in custom code). I think it is mostly due to developers not being aware of the importance or even existence of config schema. One might naively think "my config doesn't need to be translated, therefore, I don't need a schema for it".

So if there were a warning all of a sudden, maybe people would look what it is about and fix it. Maybe we need to add a link to a documentation page about config schema.

But here is a attempt to get this back on track. I think we need schema validation, but I also think first we need to raise awareness of how important config schema is.

Ok I guess it should adhere to coding standards for people to review :D

The current change record is outdated.

Oh, of course both files need to have the same content... should have known.

We should probably debate the message if we agree to take this route.

Looks good to me. I am using this approach #93 for a few years. It helped me to discover many config schema issues in custom code and contrib.

The message is a bit confusing. I know it comes from the exception but since it's now showed in admin UI can we improve wording somehow?
For people that are not familiar with Drupal Configuration system it could be hard understand what's wrong.

I expect many Drupal developers will get this message on their local hosts. So the CR should have some explanation of what configuration schema is and how to fix errors in it, probably with some links to the documentation on drupal.org.

Updated the CR, updated the message to be more helpful and also show a link to the documentation. Tests seem green. Let's see if we can continue here.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. 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.

Removed new forbidden word "please" from the message.

1 small comment but

Consider adding a todo to move it out of the Testing namespace in D9.

And decide on the namespace. There was an initial suggestion of 'Drupal\Core\Config\Schema\ConfigSchemaChecker'. And later the patch used '\Drupal\Core\Config\Devel\ConfigSchemaChecker', which was changed to '\Drupal\Core\Config\Development\ConfigSchemaChecker'.

Is the current namespace '\Drupal\Core\Config\Development\ConfigSchemaChecker' the one to use or something else?

Is the todo still needed?

Has the namespace been decided?

Added a couple small grammar suggestions.

We can decide the namespace, i think this placement is all good.

Applied suggestion for grammar and adjusted contructor.

+1 from me.

Nit: add readonly to the promoted properties. Otherwise can RTBC.

Yeah, might as well, good one.

There are test failures. Might need rerunning?

Re-run seemed to do the trick. Believe all feedback has been addressed

The CR is no longer correct as the class is not moving. I do think that the CR should exist - it should detail that by default if you use the development.services.yml you will can warnings when saving configuration that does not comply with schema. Once the CR is updated this can be set back to RTBC. The solution looks elegant and it's great that this emits warnings rather than errors.

CR has been updated.

CR looks great.

Committed 733dc5f and pushed to 11.x. Thanks!