Add validation constraints to filter.settings

This MR introduces a new Constraint that checks that an entity with a given ID and entity type exists. This is probably useful in a number of other places so it would be worth reviewing issues that might need it and linking so we're not duplicating work

The assertion that is currently failing is in ConfigImportAllTest::testInstallUninstall() on line 147

    // Check that all modules that were uninstalled are now reinstalled.
    $this->assertModules(array_keys($modules_to_uninstall), TRUE);

The test is asserting that a set of modules are enabled, and is failing because block_content is not enabled. However, debugging shows it would fail on other modules - here's a list of the enabled status of all checked modules.

action = true
announcements_feed = true
automated_cron = true
ban = true
basic_auth = true
big_pipe = true
block = true
block_content = false
book = false
breakpoint = false
ckeditor5 = false
comment = false
config_translation = false
contact = false
content_moderation = false
content_translation = false
contextual = false
datetime = false
datetime_range = false
dblog = false
dynamic_page_cache = true
editor = false
field = false
field_layout = false
field_ui = false
file = false
filter = true
forum = false
help = true
history = false
image = false
inline_form_errors = true
jsonapi = false
language = false
layout_builder = false
layout_discovery = false
link = false
locale = false
media = false
media_library = false
menu_link_content = false
menu_ui = false
migrate = false
migrate_drupal = false
migrate_drupal_ui = false
node = false
options = false
page_cache = true
path = false
pgsql = true
phpass = false
responsive_image = false
rest = false
sdc = false
search = false
serialization = false
settings_tray = false
shortcut = false
sqlite = true
statistics = false
syslog = true
experimental_module_requirements_test = true
experimental_module_test = true
system_test = true
taxonomy = false
telephone = false
text = false
toolbar = false
tour = true
update = true
views = false
views_ui = false
workflows = false
workspaces = false

I checked this, and I can't find the problem, but like @borisson_ sais, must be in the config, debugging the core.extensions.yml file has the full list of modules that must be installed, but when the syncronization is executed in the test ConfigImportAllTest lines:

$this->drupalGet('admin/config/development/configuration');
$this->submitForm([], 'Import all');

Is not installing all the modules.

I tried to debug the syncronization import in Drupal\config\Form\ConfigSync, but I can't :(

Getting the list of enabled modules before

// Check that all modules that were uninstalled are now reinstalled.
    $this->assertModules(array_keys($modules_to_uninstall), TRUE);

line, there are 26 installed modules of 74.

Also something strange, in the new validator EntityExistsConstraintValidator, adding one "return;" before add the violation (or removing the violation), the test is green and I don't understand why, because in my case, that point never is reached, the 'if' always is 'true'.

    // If we can load an entity of the given type with the ID in value we pass
    // the constraint.
    if ($this->entityTypeManager->getStorage($constraint->entityType)->load($value)) {
      return;
    }

    return;

    $violation = $this->context
      ->buildViolation($constraint->message)
      ->setParameter('@entity_type', $constraint->entityType)
      ->setParameter('%value', $value);
    $violation->addViolation();

I could not find the actual problem either.
Said that, hopefully some extra clues follow.

Also something strange, in the new validator EntityExistsConstraintValidator, adding one "return;" before add the violation (or removing the violation), the test is green and I don't understand why, because in my case, that point never is reached, the 'if' always is 'true'.

@nireneko, that is expected, a return or the non-creation of a violation means the constraint passes validation.

As stated at previous comments, there are multiple modules not enabled on the configuration import done on the failing test.

Finding the error is a bit tricky, since ConfigImportAllTest::testInstallUninstall() is using the configuration synchronization UI, and that uses the batch API.
Inside the Batch API, errors seem to not be relied, or at least not in tests, see ob_\* calls at https://git.drupalcode.org/project/drupal/-/blob/0bbf07394de4a370c28b904....

Trying to figure the problem out, I noticed that if the module installer service is used to enable the modules, they can be enabled.
E.g. adding the following change will make block_content module enabled correctly, and the error will be reported on the next not re-enabled module, book.

diff --git a/core/modules/config/tests/src/Functional/ConfigImportAllTest.php b/core/modules/config/tests/src/Functional/ConfigImportAllTest.php
index 4d693c6333..7beb8b66ea 100644
--- a/core/modules/config/tests/src/Functional/ConfigImportAllTest.php
+++ b/core/modules/config/tests/src/Functional/ConfigImportAllTest.php
@@ -139,6 +139,7 @@ public function testInstallUninstall() {
     $this->submitForm([], 'Import all');
     // Modules have been installed that have services.
     $this->rebuildContainer();
+    $this->assertTrue(\Drupal::service('module_installer')->install(['block_content']), 'Manually installing block_content actually installs it.');
 
     // Check that there are no errors.
     $this->assertSame([], $this->configImporter()->getErrors());

In other words, the code path enabling the modules with the configuration synchronization form on the test is doing something different than the call to \Drupal::service('module_installer')->install().
What exactly, not quite sure.

Overall, it may be that the new configuration validation is correctly triggering a problem, but that problem seems to only exist on the testing code path, and not on the normal code path, outside test via phpunit.

I find something weird with schema of filters.

This is text format schema (removed some parts for readability):

filter.format.*:
  type: config_entity
  mapping:
    name:
      type: required_label
    (...)
    filters:
      type: sequence
      orderby: key
      sequence:
        type: filter

Each filter is of type filter:

filter:
  type: mapping
  mapping:
    id:
      type: string
    (...)
    settings:
      type: filter_settings.[%parent.id]

Each filter's settings is of type filter_settings.<filter_plugin_id>, defaulting to filter_settings.*

But here comes the weirdness: Each filter_settings.<filter_plugin_id> is again of type filter (except filter_settings.* which correctly goes to a sequence)

For instance:

filter_settings.filter_html:
  type: filter
  mapping:
    ...

I think each filter_settings.<filter_plugin_id> should be of type mapping

Am I missing something?

I've opened #3404431: Filter settings schema types are incorrect for #14

#14: you re-discovered #3401837: Add basic validation to config schema definitions 😄

EDIT: reviewed your new issue 😊

Blocked on #3404431: Filter settings schema types are incorrect.

#3404431: Filter settings schema types are incorrect landed!

Lifted some code from #3412361: Mark Editor config schema as fully validatable to expand what the existing ConfigExists constraint can do, which is why I associated this change record with this issue.

It also means I was able to remove the new EntityExists constraint that @Eli-T proposed. I think that would be an excellent addition, but such a useful general addition is not a hard blocker for this issue. There's now two distinct issues that would benefit from that expansion in capability for ConfigExists, so went ahead with that.

What's still missing is test coverage. \Drupal\Tests\filter\Functional\FilterAdminTest::testFilterAdmin() does some testing of filter.settings, so is the closest place we have.

Aha … the reason Drupal.Tests.config.Functional.ConfigImportAllTest is not passing tests is actually quite simple:

1. filter.settings is simple config
2. ⚠️ Only config entities are allowed to have config dependencies
3. When filter.settings is installed/saved, text format config entities have not yet been installed
4. Hence ConfigImportAllTest::testInstallUninstall() fails.

There is the short-term solution and the long-term one:

Short-term solution

Make the validation less strict — do not verify the referenced text format config entity actually exists, just verify it is a valid text format config entity ID

Long-term solution

Remove filter.settings:fallback_format and instead add a is_fallback property to FilterFormat config entities?

(But how to then guarantee only one of these is marked as the fallback without introducing a race condition there? 🤔)

Because there is no clear long-term solution either, I think the short-term solution is fine for now.

Left a small comment, let me know what you think.

This flaw in filter.settings was introduced in #1799440: Convert Filter variables to Configuration System.

#1932544: Remove all traces of fallback format concept from the (admin) UI is the long-term solution.

Updated comment 👍

Committed/pushed to 11.x, thanks!