- Issue created by @Wim Leers
- Merge request !5702Resolve #3406478 "Validkeysconstraintvalidator dynamicinvalidkeymessage" β (Open) created by Wim Leers
- Status changed to Needs review
9 months ago 12:28pm 6 December 2023 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
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.
- Issue was unassigned.
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
βΉοΈ This MR contains
311 insertions(+), 60 deletions(-)
of the1125 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.
- Status changed to Needs work
9 months ago 3:24pm 6 December 2023 - πΊπΈUnited States phenaproxima Massachusetts
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..
- Assigned to Wim Leers
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
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 :)
- Issue was unassigned.
- Status changed to Needs review
9 months ago 6:51pm 6 December 2023 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
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)
- Status changed to RTBC
9 months ago 7:06pm 6 December 2023 - πΊπΈUnited States phenaproxima Massachusetts
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.
- π§πͺBelgium borisson_ Mechelen, π§πͺ
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.
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
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!
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
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 π₯³
- Status changed to Needs work
9 months ago 2:15pm 13 December 2023 - π¬π§United Kingdom alexpott πͺπΊπ
I think we need to handle dprecating the public API rather than just removing it.
- Assigned to Wim Leers
- Issue was unassigned.
- Status changed to RTBC
9 months ago 10:07am 15 December 2023 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
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. - Status changed to Needs work
9 months ago 7:55am 21 December 2023 - π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Left some questions on the MR, only one of them is important
- Status changed to Needs review
9 months ago 9:55am 21 December 2023 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
Addressed all of @larowlan's feedback π
(And merged in
11.x
, which allowed reverting 2 lines π) - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
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! - Assigned to Wim Leers
- Status changed to Needs work
9 months ago 7:00am 22 December 2023 - Issue was unassigned.
- Status changed to RTBC
9 months ago 7:44am 22 December 2023 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
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.
- Status changed to Fixed
9 months ago 8:46am 25 December 2023 -
alexpott β
committed c980ece6 on 11.x
Issue #3406478 by Wim Leers, phenaproxima, borisson_, alexpott, larowlan...
-
alexpott β
committed c980ece6 on 11.x
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
Yay! Next up: #3364108-90: Configuration schema & required keys β π
Automatically closed - issue fixed for 2 weeks with no activity.