[Validator] Allow objects implementing __toString() to be used as violation messages #31083

mdlutz24 · 2019-04-12T02:28:21Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR	I didn't see a doc on violations to update, but I'm happy to do documentation if somone can suggest the best place to do it.

Currently in the Drupal project we use Translatable Markup object to hold most strings and currently pass them in as Constraint Violation messages. In Symfony 3 this works but with the added typehinting in Symfony 4, these markup objects are rendered into strings at the time of the violation creation. This causes any html in the message string to be considered unsafe by twig later in our rendering process. This pr explicitly allows objects implementing a __toString() method to be used as violation messages, and the violation will save and return the original stringable object.

See https://www.drupal.org/project/drupal/issues/3029540 For our Drupal issue on the subject.

src/Symfony/Component/Validator/ConstraintViolationInterface.php

alexpott · 2019-04-12T20:05:45Z

@michaelcullum and I talked a bit around this issue and whether or not we should be pushing for a Stringable pseudo-class in PHP - there are prior discussions - see https://externals.io/message/98424

xabbuh · 2019-04-13T09:05:07Z

I am 👎 on making this change as it calls for breaking existing code that do not perform explicit string casts on the returned value of getMessage().

nicolas-grekas · 2019-04-13T09:18:50Z

What are the alternatives on your side @alexpott @mdlutz24?

alexpott · 2019-04-17T09:53:40Z

@nicolas-grekas none of them are particularly pretty - https://www.drupal.org/project/drupal/issues/3029540 - we've explored quite a few there.

ro0NL · 2019-04-17T10:07:13Z

does the issue affect drupal users directly? (if not i think drupal core should just patch)

e.g.

symfony/src/Symfony/Component/Validator/Context/ExecutionContextInterface.php

Lines 67 to 70 in 59c08ed

    
                * @param string $message The error message 
        
                * @param array  $params  The parameters substituted in the error message 
        
                */ 
        
               public function addViolation($message, array $params = []);

is the issue drupal users do add/buildViolation(t('message')) at this point? Is it possible for drupal to provide its own ExecutionContextInterface?

nicolas-grekas

We have to do this change. Let's be friendly to our users. It's the only pragmatic move.
Here are some CS changes before approving on my side.

src/Symfony/Component/Validator/ConstraintViolation.php

src/Symfony/Component/Validator/ConstraintViolationInterface.php

nicolas-grekas · 2019-07-30T19:28:14Z

(please rebase also, on branch 4.4)

ro0NL · 2019-07-30T19:49:00Z

note in 5.0 there are more typehints that could be blocking still;

symfony/src/Symfony/Component/Validator/Context/ExecutionContextInterface.php

Line 70 in 1f8f4f2

public function addViolation(string $message, array $params = []);

symfony/src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php

Line 46 in 1f8f4f2

    
           public function __construct(ConstraintViolationList $violations, Constraint $constraint, string $message, array $parameters, $root, $propertyPath, $invalidValue, TranslatorInterface $translator, $translationDomain = null)

nicolas-grekas · 2019-07-30T19:50:44Z

@ro0NL thanks, then let's add as a string or a stringable object to all places where this would block lazy messages, in 4.4. IMHO.

nicolas-grekas

please have a look at tests :)

src/Symfony/Component/Validator/ConstraintViolation.php

src/Symfony/Component/Validator/ConstraintViolationInterface.php

src/Symfony/Component/Validator/CHANGELOG.md

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php

nicolas-grekas

Good on my side (just one very minor CS comment)
Thanks for being so reactive.

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php

nicolas-grekas · 2019-08-08T16:31:05Z

(rebase needed)

…lation messages [Validator] updated changelog [Validator] updated typehint for ConstraintViolationInterface::getMessage() [Validator] fixed spacing issue inadvertantly added in previous commit [Validator] fixed coding standard issues [Validator] Address feedback [Validator] Fix coding standard violation [Validator] update tests [Validator] Address feedback, Round 2 [Validator] Document ConstraintViolationBuilder::__construct() parameter [Validator] Update changelog [Validator] Adjust parameter documentation order

mdlutz24 · 2019-08-08T17:21:29Z

Rebased and squashed

fabpot · 2019-08-09T06:11:07Z

Thank you @mdlutz24.

… be used as violation messages (mdlutz24) This PR was merged into the 4.4 branch. Discussion ---------- [Validator] Allow objects implementing __toString() to be used as violation messages | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | I didn't see a doc on violations to update, but I'm happy to do documentation if somone can suggest the best place to do it. Currently in the Drupal project we use Translatable Markup object to hold most strings and currently pass them in as Constraint Violation messages. In Symfony 3 this works but with the added typehinting in Symfony 4, these markup objects are rendered into strings at the time of the violation creation. This causes any html in the message string to be considered unsafe by twig later in our rendering process. This pr explicitly allows objects implementing a __toString() method to be used as violation messages, and the violation will save and return the original stringable object. See https://www.drupal.org/project/drupal/issues/3029540 For our Drupal issue on the subject. Commits ------- 79f4dcd [Validator] Allow objects implementing __toString() to be used as violation messages

carsonbot added Status: Needs Review Feature labels Apr 12, 2019

mdlutz24 changed the title ~~Stringableviolations~~ [Validator] Allow objects implementing __toString() to be used as violation messages Apr 12, 2019

ro0NL reviewed Apr 12, 2019

View reviewed changes

src/Symfony/Component/Validator/ConstraintViolationInterface.php Outdated Show resolved Hide resolved

xabbuh added the Validator label Apr 12, 2019

xabbuh added this to the next milestone Apr 12, 2019

nicolas-grekas changed the base branch from master to 4.4 July 30, 2019 19:22

nicolas-grekas reviewed Jul 30, 2019

View reviewed changes

mdlutz24 force-pushed the stringableviolations branch from 59c08ed to 58424ea Compare July 30, 2019 20:59

nicolas-grekas reviewed Jul 30, 2019

View reviewed changes

src/Symfony/Component/Validator/ConstraintViolation.php Outdated Show resolved Hide resolved

src/Symfony/Component/Validator/ConstraintViolationInterface.php Show resolved Hide resolved

nicolas-grekas reviewed Jul 31, 2019

View reviewed changes

src/Symfony/Component/Validator/CHANGELOG.md Outdated Show resolved Hide resolved

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php Show resolved Hide resolved

nicolas-grekas approved these changes Jul 31, 2019

View reviewed changes

src/Symfony/Component/Validator/Violation/ConstraintViolationBuilder.php Show resolved Hide resolved

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jul 31, 2019

mdlutz24 force-pushed the stringableviolations branch from a0da99d to 79f4dcd Compare August 8, 2019 17:19

fabpot approved these changes Aug 9, 2019

View reviewed changes

fabpot merged commit 79f4dcd into symfony:4.4 Aug 9, 2019

fabpot mentioned this pull request Aug 9, 2019

[Validator] Allow objects implementing __toString() to be used as viola… symfony/symfony-docs#12123

Closed

nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019

This was referenced Nov 12, 2019

Release v4.4.0-BETA1 #34338

Merged

Release v5.0.0-BETA1 #34339

Merged

ro0NL mentioned this pull request Nov 29, 2019

BC break in return type of ConstraintViolationInterface::getMessage() #34710

Closed

nicolas-grekas mentioned this pull request May 12, 2020

[Validator] clarify stringable type annotations #36057

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Validator] Allow objects implementing __toString() to be used as violation messages #31083

[Validator] Allow objects implementing __toString() to be used as violation messages #31083

mdlutz24 commented Apr 12, 2019

alexpott commented Apr 12, 2019

xabbuh commented Apr 13, 2019 •

edited

Loading

nicolas-grekas commented Apr 13, 2019

alexpott commented Apr 17, 2019

ro0NL commented Apr 17, 2019

nicolas-grekas left a comment

nicolas-grekas commented Jul 30, 2019

ro0NL commented Jul 30, 2019

nicolas-grekas commented Jul 30, 2019 •

edited

Loading

nicolas-grekas left a comment

nicolas-grekas left a comment

nicolas-grekas commented Aug 8, 2019

mdlutz24 commented Aug 8, 2019

fabpot commented Aug 9, 2019

[Validator] Allow objects implementing __toString() to be used as violation messages #31083

[Validator] Allow objects implementing __toString() to be used as violation messages #31083

Conversation

mdlutz24 commented Apr 12, 2019

alexpott commented Apr 12, 2019

xabbuh commented Apr 13, 2019 • edited Loading

nicolas-grekas commented Apr 13, 2019

alexpott commented Apr 17, 2019

ro0NL commented Apr 17, 2019

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Jul 30, 2019

ro0NL commented Jul 30, 2019

nicolas-grekas commented Jul 30, 2019 • edited Loading

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas left a comment

Choose a reason for hiding this comment

nicolas-grekas commented Aug 8, 2019

mdlutz24 commented Aug 8, 2019

fabpot commented Aug 9, 2019

xabbuh commented Apr 13, 2019 •

edited

Loading

nicolas-grekas commented Jul 30, 2019 •

edited

Loading