Refine ValidKeysConstraintValidator's messages: use #3401883's new Mapping infrastructure

The updated test coverage should fail like this:

Testing /Users/wim.leers/core/core/tests/Drupal/KernelTests/Core/TypedData
.F..                                                                4 / 4 (100%)

Time: 00:04.529, Memory: 10.00 MB

There was 1 failure:

1) Drupal\KernelTests\Core\TypedData\ValidKeysConstraintValidatorTest::testUnknownKeys
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
-    0 => ''use_site_logo' is an unknown key because plugin is system_powered_by_block (see config schema type block.settings.*).'
-    1 => ''use_site_name' is an unknown key because plugin is system_powered_by_block (see config schema type block.settings.*).'
-    2 => ''use_site_slogan' is an unknown key because plugin is system_powered_by_block (see config schema type block.settings.*).'
+    0 => ''use_site_logo' is not a supported key.'
+    1 => ''use_site_name' is not a supported key.'
+    2 => ''use_site_slogan' is not a supported key.'
 )

So:

In other words: a much more detailed message.

ℹ️ This MR contains 311 insertions(+), 60 deletions(-) of the 1125 insertions(+), 151 deletions(-) that the MR for 📌 Configuration schema & required keys Fixed contains.

IOW: this shrinks the other MR by ~30%. The other MR can be layered cleanly on top of this one; not a single line added there would need to change.

This looks good and makes sense. I have a fair number of small comments (as I usually do), in the hope of improving clarity, but overall I find this close to RTBC-able..

This looks good and makes sense. I have a fair number of small comments (as I usually do), in the hope of improving clarity, but overall I find this close to RTBC-able..

I would hope so, since you've already reviewed exactly this code multiple times in 📌 Configuration schema & required keys Fixed 😄

On it :)

Some good questions about code clarity, I hope I made you proud, @phenaproxima 😄 (This ended up taking 1.5 hour 🤯 — naming & explaining can be time-consuming :D)

I don't think I have any real problems with this. The really complicated and potentially confusing stuff is now heavily commented with examples, the variable names are probably about as as clear as they can be, and the test coverage makes sense too. This is a worthy improvement to the ValidKeys constraint and will be really helpful for DX.

We could go back-and-forth on this for longer if we wanted to, but we've got bigger fish to fry. Ship it.

When this issue was opened earlier today, it was my plan to take some time after to work to look into this, even though I already reviewed the parent MR a couple of times.
It looks like @phenaproxima and @Wim Leers have already taken the fun out of the review, since it is already improved over the orignal with much more/better comments and better naming.

If this lands before 🐛 Consistently use "dynamic type name" and "expression" instead of "variable value" in TypedConfigManager's terminology Fixed , we have to make sure to apply the suggestions that @phenaproxima made here to that one, but that should be easy enough to do.

Feels like all I can say is that I agree with the rtbc in #8.

It looks like @phenaproxima and @Wim Leers have already taken the fun out of the review, since it is already improved over the orignal with much more/better comments and better naming.

🤣

You've both already heavily influenced this MR, by reviewing 📌 Configuration schema & required keys Fixed as much as you have 😊 Thank you!

Applied all of @phenaproxima's remaining suggestions because 🐛 Consistently use "dynamic type name" and "expression" instead of "variable value" in TypedConfigManager's terminology Fixed landed 🥳

I think we need to handle dprecating the public API rather than just removing it.

Restored ValidKeysConstraint::getAllowedKeys() method per @alexpott's review. Since this is just trivially moving logic from one spot to another, there's 0 functional changes and tests are still passing: back to RTBC.

Left some questions on the MR, only one of them is important

Addressed all of @larowlan's feedback 😊

(And merged in 11.x, which allowed reverting 2 lines 👍)

I merged in upstream in #16. This caused a failure in \Drupal\Tests\block\Kernel\Migrate\d6\MigrateBlockTest due to 📌 Remove forum from block migration tests Fixed (landed 2 days ago), whose expected messages become more precise thanks to this issue. 👍 Trivial fix!

Based on @larowlan's feedback, I reverted a few lines from the test coverage additions (because they belong in 📌 Configuration schema & required keys Fixed , which is blocked by this issue, plus Lee marked the MR thread as resolved himself), added a typehint and tweaked an assert().

The actual changes are so minor that I feel a re-self-RTBC is reasonable.

Committed c980ece and pushed to 11.x. Thanks!

Yay! Next up: #3364108-90: Configuration schema & required keys → 😊

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