Adopt PluginExists validator in relevant places

Just moving to NW as this sounds like it's ready to be picked up. You guys put together a great roadmap.

ConfigSchemaTest::testSchemaMapping needs updating, it was expected to fail here, the other changes already look great.

We don't need to handle any kind of migration plugins; these are never represented in config.

Marking all the Help and Help Topics plugins as N/A, since I don't see them mentioned or implied anywhere in config schema.

As far as I know, menu link plugins are not encoded anywhere in core's config, so I'm marking all of those N/A.

The words "join" and "wizard" are nowhere in Views' config directory, so I'm marking those plugin types as N/A.

I don't think LB section storages are mentioned in config anywhere, at least not in core, so I'm marking that plugin type N/A.

Looking great! And surprisingly few config bugs 👍

It seems that every single one of the remaining strings in config that are actually plugin IDs is being updated to have the necessary validation … so … I think that means that this is in principle RTBC'able? 😄🤞

Is

Write test that verifies that all default config in Drupal core complies with the validation constraints, by adding a new testValidity() method to \Drupal\KernelTests\Core\Config\DefaultConfigTest

in the IS still relevant?

I agree with #29, this is in a good state, it needs an issue summary update, but already rtbc based on the patch

Tagging per #30.

Moving to NW for the issue summary update.

This has undergone some significant changes, because I discovered that we need to handle fallback plugin IDs in the PluginExists constraint validator. I uncovered this while trying to write test coverage for views.

Fallback plugin IDs are problematic from a config validation perspective. In general, I think fallback plugin IDs are meant to deal with unexpected missing plugin definitions at runtime. But by that same token, valid config should (almost) never contain invalid plugin IDs, and the validator should not, by default, accept a fallback plugin ID as valid.

There are a couple of cases where that makes sense -- for example, block entities, which might reference a block_content:NNNN plugin that doesn't exist yet -- but generally it doesn't. Core is littered with test views (and indeed, a couple of "real" ones) that contain invalid plugin IDs. Rather than fix them here, I decided to allow the PluginExists constraint validator to accept fallback plugin IDs in certain situations. We'll need a follow-up to fix all of those places and make the config schema stricter (by removing most of the allowFallback: true lines).

Having said all that, I'd like review on this updated code before finalizing the issue summary and opening the follow-up.

I agree with the approach in #33, it makes to allow fallbacks for now and remove them in a followup in the cases where they aren't helpful.

I read through the PR, and the only remark I have is that all todo's need to have a d.o issue id linked as well, but you already mentioned that.
In my opinion this is conceptually rtbc.

Opened a follow-up to fix Views' config schema.

I think we'll probably need a follow-up for validating Views display extenders, since that will (apparently) require the TypeResolver stuff being added by 📌 Add new `EntityBundleExists` constraint Fixed . Either that, or we should block this issue on that one.

We still need this!

This needs a rebase now.

I merged 11.x back into this branch, there are a couple of test failures now.

Seems like most (perhaps all) test failures are due to the use of random and hence non-existing plugin IDs in test coverage. Those will need to change to use actual plugin IDs. IOW: our test coverage will become more realistic 👍😄

Back to you!

There are still 2 open comments on the MR, can we address those as well?

Agreed with @borisson_.

The follow-up you mentioned we need in #33 is still necessary too 😄🙏

Opened 🐛 [PP-1] In most situations, config that references plugins should not be allowed to reference fallback plugin IDs Postponed and addressed the final two bits of feedback.

Thank you!

Any chance you forgot to push 1 commit? The thread https://git.drupalcode.org/project/drupal/-/merge_requests/4534?commit_i... is marked resolved but EditorValidationTest is not yet being changed by this MR?

Apparently I'd already opened the follow-up: 🐛 [PP-1] Views' config schema should not consider fallback plugin IDs as valid Postponed . Closing the new one as a duplicate.

Re #46, I did not forget to push. :) The test coverage you're asking for already exists.

Aha, it was added in ✨ Add config validation for plugin IDs Fixed on October 1, and I had requested that test coverage on August 22 😅 Sorry for the confusion!

Needs a rebase. I updated credit.

Is the Issue Summary still correct?

Just saving my work-in-progress; I'm removing the table and turning it into a series of lists for clarity.

There are two follow-ups needed here.

There's a todo in field.field_settings.entity_reference:handler:

          # @todo Remove this line and explicitly require valid entity reference
          # selection plugin IDs in core wherever it makes sense.
          allowFallback: true

We'll need a separate issue for that, linked there.

And there's this piece of the issue summary:

There is one Views plugin type, \Drupal\views\Plugin\views\display_extender\DisplayExtenderPluginBase, which is not addressed by this issue because it is not used anywhere in core, so it's unclear how, or if, these plugin IDs are stored in config.

Added 🐛 [PP-1] Entity reference fields' config schema should not be allowed to refer to the fallback selection plugin Postponed as a follow-up for dealing with the fact that, for now, entity reference fields' config schema considers fallback plugin IDs as valid. It's blocked on this issue.

Added 🐛 [PP-1] How should we validate Views display extenders in config schema? Postponed for display extenders.

Follow-ups are filed and no substantive changes have been needed to the patch itself, so I'm restoring RTBC here!

Agreed with re-RTBC — the only change to the code has been an updated @todo (which now links to an issue 👍)

The @todos towards getting rid of Views fallback handlers cheered me up, one day we will get there and not give people horrible surprises when they edit a view potentially months after it broke.

Committed/pushed to 11.x, thanks!

Automatically closed - issue fixed for 2 weeks with no activity.