- Issue created by @wim leers
- Status changed to Needs review
about 1 year ago 7:07am 18 October 2023 - last update
about 1 year ago CI aborted - π§πͺBelgium borisson_ Mechelen, π§πͺ
Implementation discussed with @Wim Leers yesterday.
- Status changed to Needs work
about 1 year ago 7:55am 18 October 2023 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
-
+++ b/core/lib/Drupal/Core/Config/Schema/TypeResolver.php @@ -84,8 +84,17 @@ private static function replaceVariable(string $value, $data) { + $previous_name = null;
s/null/NULL/
-
+++ b/core/lib/Drupal/Core/Config/Schema/TypeResolver.php @@ -84,8 +84,17 @@ private static function replaceVariable(string $value, $data) { + throw new \LogicException('The only valid usages of a config schema with a % in it are %parent, %key, and %type in ' . $value);
\Drupal\Core\Config\TypedConfigManager::replaceVariable()
calls these variable values, we should probably do the same here. -
+++ b/core/lib/Drupal/Core/Config/Schema/TypeResolver.php @@ -84,8 +84,17 @@ private static function replaceVariable(string $value, $data) { + throw new \LogicException('%type can only used when immediatly preceeded by %parent in ' . $value);
Typos π
-
- last update
about 1 year ago Patch Failed to Apply - last update
about 1 year ago Patch Failed to Apply - Status changed to Postponed
12 months ago 8:10am 3 December 2023 - π§πͺBelgium borisson_ Mechelen, π§πͺ
Was looking at the configuration validation issues, and noticed this should actually be marked as postponed, changing status to match the reality.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
This would complement π Consistently use "dynamic type name" and "expression" instead of "variable value" in TypedConfigManager's terminology Fixed very well.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π Consistently use "dynamic type name" and "expression" instead of "variable value" in TypedConfigManager's terminology Fixed landed!
@borisson_: Could we add a secondary MR here that does not modify
TypeResolver
(which does not exist yet, which is why this issue is postponed), but instead modifiesTypedConfigManager
? We'd have to rebase theEntityBundleExists
MR that this is blocked on, but we probably need to rebase it anyway at this point. - Assigned to wim leers
- Status changed to Active
10 months ago 8:35am 31 January 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π Add new `EntityBundleExists` constraint Fixed landed so this is now unblocked!
- Merge request !6404Resolve #3392903 "Validate inputs resolveexpression" β (Open) created by wim leers
- Status changed to Needs review
10 months ago 8:46am 31 January 2024 - Issue was unassigned.
- Status changed to Needs work
10 months ago 8:50am 31 January 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
FWIW, this is becoming more important, because other issues are starting to adopt
TypeResolver
too β for example π [PP-1] Convert field_storage_config and field_config's form validation logic to validation constraints Postponed . - Status changed to Needs review
9 months ago 10:33pm 10 February 2024 - π§πͺBelgium borisson_ Mechelen, π§πͺ
Fixed the remarks from @Wim Leers. I had to update one existing test: https://git.drupalcode.org/project/drupal/-/merge_requests/6404/diffs?co...
- Status changed to Needs work
9 months ago 3:33pm 11 February 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Looking close, two small pieces of feedback π
- π§πͺBelgium borisson_ Mechelen, π§πͺ
Fixed the first (variable value) remark, the other one is a bit harder to figure out. It's part of the test coverage we created for π Add new `EntityBundleExists` constraint Fixed , not for this issue.
I don't know how to add the requested test coverage. - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@borisson_ You'll want to modify the test logic to not just mock a parent level, but a grandparent level. That way you'd be able to at least test
%parent.%parent.entity_type_id
, and then there's >1 test case again :) - Status changed to Needs review
8 months ago 1:30pm 8 March 2024 - π§πͺBelgium borisson_ Mechelen, π§πͺ
Fixed the feedback, rerolled on 11.x and added test coverage for %key. I think this is now ready for another round of reviews.
- Status changed to Needs work
8 months ago 5:54pm 8 March 2024 - Status changed to Needs review
8 months ago 8:58am 9 March 2024 - π§πͺBelgium borisson_ Mechelen, π§πͺ
Hopefully resolved the new comments.
- Status changed to Needs work
8 months ago 8:03am 11 March 2024 - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Was going to RTBC, then spotted one last small problem. π«£
Updated the issue summary for you :)
- Status changed to Needs review
8 months ago 8:22pm 11 March 2024 - π§πͺBelgium borisson_ Mechelen, π§πͺ
Applied Wim's latest suggestion, it is indeed more clear like this.
- Status changed to RTBC
8 months ago 9:52pm 11 March 2024 - πΊπΈUnited States phenaproxima Massachusetts
alexpott β credited phenaproxima β .
- π¬π§United Kingdom alexpott πͺπΊπ
Committed and pushed 94e4d4715c to 11.x and fce65ce3aa to 10.3.x. Thanks!
- Status changed to Fixed
8 months ago 10:25am 25 March 2024 -
alexpott β
committed fce65ce3 on 10.3.x
Issue #3392903 by borisson_, Wim Leers, phenaproxima: Validate inputs of...
-
alexpott β
committed fce65ce3 on 10.3.x
-
alexpott β
committed 585650c6 on 11.x
Issue #3392903 by borisson_, Wim Leers, phenaproxima: Validate inputs of...
-
alexpott β
committed 585650c6 on 11.x
Automatically closed - issue fixed for 2 weeks with no activity.