- Issue created by @Wim Leers
- Issue was unassigned.
- Status changed to Needs review
10 months ago 4:32pm 24 November 2023 - Assigned to Wim Leers
- Status changed to Needs work
10 months ago 5:35pm 24 November 2023 - First commit to issue fork.
- Issue was unassigned.
- Status changed to Needs review
10 months ago 12:11pm 27 November 2023 - Status changed to RTBC
10 months ago 12:40pm 27 November 2023 - π§πͺBelgium borisson_ Mechelen, π§πͺ
Looks great, we have sufficient coverage and this is a small bugfix.
- Status changed to Needs review
9 months ago 4:08pm 28 November 2023 - π¬π§United Kingdom alexpott πͺπΊπ
One of the kernel tests is failing do to this change... see https://git.drupalcode.org/issue/drupal-3404061/-/pipelines/56281/test_r...
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
Forgot to update that one. Green again π
- Status changed to RTBC
9 months ago 2:34pm 29 November 2023 - πΊπΈUnited States phenaproxima Massachusetts
smustgrave β credited phenaproxima β .
- πΊπΈUnited States smustgrave
Reran the failing javascript test and it passed so it was random.
1) Drupal\KernelTests\Config\TypedConfigTest::testSimpleConfigValidation Failed asserting that two strings are equal. --- Expected +++ Actual @@ @@ -'This value should not be null.' +'This value should not be blank.' /builds/issue/drupal-3404061/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94 /builds/issue/drupal-3404061/core/tests/Drupal/KernelTests/Config/TypedConfigTest.php:174 /builds/issue/drupal-3404061/vendor/phpunit/phpunit/src/Framework/TestResult.php:728 FAILURES! Tests: 2, Assertions: 47, Failures: 1.
Ran the test-only feature and got this so test coverage is there.
Crediting @phenaproxima for the MR review but his name wasn't appearing on the ticket.
Seems all feedback has been addressed though.
- π§πͺBelgium borisson_ Mechelen, π§πͺ
Agreeing with @smustgrave, all feedback has been addressed, and this is a good improvement.
- Status changed to Needs work
9 months ago 11:47am 5 December 2023 - π¬π§United Kingdom alexpott πͺπΊπ
@longwave, @Wim Leers and I have discussed this issue at length. The discussion resulted in wanting to explore some other options, namely:
1. Do it at a different level, for example \Drupal\Core\TypedData\Validation\TypedDataMetadata::getConstraints(), which is what the validation system always calls β and detect presence of both and configure the other one correctly
2. Override NotBlankValidator and just make allowNull: true do nothing.
3. Look into \Symfony\Component\Validator\Constraints\Sequentially
4. Change getDefaultConstraints() to configured NotBlank correctly instead of adding NotNull when $definition->isRequired() and NotBlank is present already. - Status changed to Needs review
9 months ago 11:55am 5 December 2023 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
π Thanks, @alexpott!
Just pushed an implementation of option 1.
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
I just tried implementing
Sequentially
.Looks like my fear was unfounded: the "unpredictable ripple effects" did not happen, thanks to
\Drupal\Core\TypedData\Validation\RecursiveContextualValidator::validate()
being very narrow in what it accepts:public function validate($data, $constraints = NULL, $groups = NULL, $is_root_call = TRUE): static { if (isset($groups)) { throw new \LogicException('Passing custom groups is not supported.'); } if (!$data instanceof TypedDataInterface) { throw new \InvalidArgumentException('The passed value must be a typed data object.'); }
β¦ because
\Symfony\Component\Validator\Constraints\SequentiallyValidator::validate()
calls Drupal'sRecursiveContextualValidator::validate()
(the first ~10 lines of which are displayed above), and$data === 'en'
at this point (for the langcode), it throws the exception with theThe passed value must be a typed data object.
message.IOW: to adopt Symfony's
Sequentially
, we need to revise a lot about how Drupal uses thesymfony/validator
component. For π Entity + Field + Property validation constraints are processed in the incorrect order Needs work we probably will need validation groups, which as you can see above are also explicitly not supported. - Assigned to Wim Leers
- Status changed to Needs work
9 months ago 12:42pm 5 December 2023 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
alexpott 32 minutes ago
I think option 4 is better than option 1.alexpott 31 minutes ago
I think we should be doing as little as possible as changing you constraints is quite surprising.Will figure out how to make that work π
- Issue was unassigned.
- Status changed to Needs review
9 months ago 4:08pm 5 December 2023 - π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
Implemented option 4.
Due to
public function getConstraints() { $constraints = $this->definition['constraints'] ?? []; $constraints += $this->getTypedDataManager()->getDefaultConstraints($this); β¦
any change I make in
getDefaultConstraints()
gets overwritten automatically.So this change must happen in the quoted code (
\Drupal\Core\TypedData\DataDefinition::getConstraints()
). - πΊπΈUnited States phenaproxima Massachusetts
Only one comment about a comment, but otherwise this looks right to me and I will RTBC it once my feedback is addressed in some way. :)
- Status changed to RTBC
9 months ago 6:27pm 5 December 2023 - Status changed to Fixed
9 months ago 11:07am 6 December 2023 -
alexpott β
committed 5cd249e9 on 11.x
Issue #3404061 by Wim Leers, borisson_, alexpott, phenaproxima, longwave...
-
alexpott β
committed 5cd249e9 on 11.x
- π§πͺBelgium Wim Leers Ghent π§πͺπͺπΊ
Thanks!
This unblocked π Configuration schema & required values: add test coverage for `nullable: true` validation support Fixed but also simplified #3379091 (see #3379091-25: [PP-1] Make NodeType config entities fully validatable β ) as well as any future work on adding validation constraints where
NotBlank
is relevant! π Automatically closed - issue fixed for 2 weeks with no activity.