From #3275864: Update to Symfony 6.1.1.
Full test results: https://www.drupal.org/pift-ci-job/2364329
1x: The "Symfony\Component\Validator\Constraints\Range::$minMessage" property is considered final. You should not override it in "Drupal\Core\Validation\Plugin\Validation\Constraint\RangeConstraint".
1x in ContextDefinitionIsSatisfiedTest::testIsSatisfiedBy from Drupal\Tests\Core\Plugin\Context
1x: The "Symfony\Component\Validator\Constraints\Range::$maxMessage" property is considered final. You should not override it in "Drupal\Core\Validation\Plugin\Validation\Constraint\RangeConstraint".
1x in ContextDefinitionIsSatisfiedTest::testIsSatisfiedBy from Drupal\Tests\Core\Plugin\Context
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
mondrakeI cannot understand the source of this deprecation. The properties are public in the parent class in Symfony, and the class is not final. Other Drupal constraint validators extending from Symfony are apparently OK. Can this be a false positive somewhere?
Comment #3
catchThis was introduced in symfony commit
751eea72a7adec0495b5Every public or protected property in any Symfony class is considered final as of Symfony 6.1. There is no language support for final properties, so they have implemented it in the debug class loader.
For this issue we have a couple of choices:
1. Set the property value in a constructor.
2. Fork the entire class instead of subclassing.
3. Suppress the message in the deprecation listener and open a follow-up to figure out whether to do #1 or #2.
If #1 works, that is probably the least change, and we could still open a follow-up to decide whether to fork the whole thing instead.
Comment #4
mondrakeLet's try #3.1, it seems supported in Symfony 6.0 already. On it.
Comment #6
catchComment #7
catchMR looks good, although seems more likely that they'll change the constructor arguments than the properties so I don't really understand the point of the new policy.
Opened #3276505: As of Symfony 6.1, all class properties are considered final.
Comment #8
mondrakeBack to NR, the previous test run failed and it was needed to check the content of the $options array before calling the parent.
Comment #9
mondrakeComment #10
mondrakeIt seems something is wrong in the entity validation code. FieldCrudTest::testFieldPropertyConstraints fails because if you're passing both a $min and a $max allowed values to the validation constraint, you should not be passing $minMessage and $maxMessage, but rather a $notInRangeMessage. In the test the min and max are visible, but the code return a 'below the min' validation error, not a 'not in range' one. Because min and max are passed, we cannot set the corresponding overriding messages, therefore code falls back to the Symfony default text.
Not sure how to tackle this - probably it's better to fall back to option 3 from #3, silence the deprecation and then work on the override along the lines of option #2.
Comment #11
mondrakeActually I think I found a way to fix this and at the same time improve the entity validator for ranges - that in this case was missing the range check that Symfony has.
Comment #12
spokjeI like the fact that our range validator for entities gets more precise with this.
The only thing that keeps me from RTBC-ing is that since we got more precise (yay!), we now throw a more precise message (yay!).
Could that affect Contrib in anyway?
I tried searching for "This value should be" on GitLab (hhttps://git.drupalcode.org/search?repository_ref=&scope=blobs&search=%22...) and that seemed OK to me.
Could anyone reassure my (most probably unfounded) apocalyptic concern isn't valid?
Comment #13
catchI think that's a good case for adding a release notes mention and change record, but not a problem for landing the patch.
Comment #14
spokjeDiscussed this with @mondrake in Slack (https://drupal.slack.com/archives/C1BMUQ9U6/p1651398561755479).
Thanks @catch for reassuring the reassurance received in there :) RTBC for me.
Comment #15
spokjeRestoring tags *sigh*
Comment #16
mondrakeAdded release note snippet.
Comment #17
mondrakeDraft CR https://www.drupal.org/node/3278195
Comment #18
mondrakererolled
Comment #19
alexpottCommitted 64aebe1 and pushed to 10.0.x. Thanks!
Comment #23
xjmThe release note does not describe a disruptive change. Can we clarify how contrib would be impacted, and who would be impacted?
The release note should also link the change record (with unique link text that is meaningful out of context).
Comment #24
longwaveIs there any need for a release note? To me this seems the equivalent of a string change, forced by upstream code, and we allow those in minors even without notification in most cases.
Comment #25
catchAgreed with @longwave, it's not really disruptive so we can just drop the release note here.