[Symfony 4] Sub class \Symfony\Component\Validator\ConstraintViolation and use that in \Drupal\Core\TypedData\Validation\ExecutionContext::addViolation()

Lets try this...

+++ b/core/lib/Drupal/Core/Validation/ConstraintViolation.php
@@ -0,0 +1,60 @@
+    // TODO: Change the autogenerated stub.

I need to fix this one. Anything else I missed?

I'm glad it was that easy, I wasn't sure it would fly like that.

re #7:

So, we don't use ->getMessage() in a lot of places, so I think we can do one of two things, either extend the ConstraintViolationInterface and add a getMessageMarkup() : MarkupInterface|string method and refactor to use that to preserve and return the original markup, or go back to using the symfony ConstraintViolation and rebuild the message as TranslateableMarkup using ->getMessageTemplate() and ->getParameters() prior to display.

@larowlan I noticed the errant @todo in #5, but I'm looking for FM answers to the questions in #8 and #10 before I rework the patch.

Should we be using this class somewhere? Like in the validator? Feels like that is missing from the patch

I'm not sure what this is referring to. I replaced all half a dozen or so usages of the symphony ConstraintViolation class with our overridden version in this patch. It's not used anywhere else in core code.

The constructor isn't included in the interface, so rather than trying to collect the parameters for TranslateableMarkup in the constructor and potentially rebuild it by incorporating getTranslatableMarkup(), can't we just extend the constructor to accept string|Markup, store whatever it gets, and return it through a new function on our extended interface getOriginalMessage() to just return what was originally passed, be it markup or string? We can then start using that function in core, trigger_error on getMessage but pass the original back through for now.

It is proving more difficult to extend ConstraintViolationInterface than I thought. The symfony version of the interface is returned by Symfony's ConstraintViolationListInterface->get(), which we use and extend, and it is proving a pain to solidly enforce an extended interface correctly. I hate to have to add a bunch of instanceof checks around, but if we extend the interface we may have to.

Throwing something against the testbot. I basically implemented #18, and did a typecheck around all the calls to ->getMessage() outside of tests. It turned out to be not as bad as I thought it might be.

I'm honestly not sure what this might break.
If it passes and the approach is approved by the FMs, this will likely need additional tests to make sure it's BC with anything in contrib still using Symfony ConstraintViolation

It would seem that I somehow missed a large number of calls to ConstraintViolation->getMessage() in my initial search. I'll take a look and see how many. It may make this approach uglier than I thought.

Yup, it got ugly, and I'm not sure this fixes all of them. :-(

Well, for completion's sake, here's the last test fixed, plus some coding standards.

Rerolling with updates for JSONAPI

A couple more from revisionable taxonomy and menu links

I'm starting to think the best thing to do is go with this basic approach, but not trigger an error on ->get message() , or only trigger it if the message is actually markup. We document that getMessage will typecast to string and render and filter, and let calling code decide if it needs it, then use getOriginalMessage only if we need to preserve markup. This should reduce the patch size and make it less ugly.

Hoping to move this forward again, as it's the last difficult issue preventing Symfony 4 support.

Unfortunatly the interdiff here isn't much help because I changed ->getOriginalMessage to ->getMarkup for clarity, but here are the highlights of the patch:

Changed the new ConstraintViolationInterface to MarkupConstraintViolationInterface, which no longer extends Symfony's ConstraintViolationInterface.
The interface has 2 methods:

• getMarkup(), which returns the message in Markup form. It returns the original Markup object if it was created with one, or a new one wrapping the message if it was created with a string.
• isMarkup(), which returns a bool indicating whether the violation was created with a Markup object or not.

The new ConstraintViolation implements both of those methods, and an additional method with the signature public static function extractMessage(\Symfony\Component\Validator\ConstraintViolation $violation)

This static method takes care of determining if we are dealing with one of our violations or a symfony style violation, and whether it was created with a string message or a Markup message, and returns the correct format.

The new ConstraintViolation also overrides ->getMessage() and throws a deprecation error if the violation was originally created with Markup.

Our ConstraintViolation still replaces Symfony's throughout core, all calls to ->getMessage() in core code have been replaced with ConstraintViolation::extractMessage($violation) as a BC layer in case external code is still using Symfony violations. This is at least somewhat cleaner than adding all the instanceof checks like I was doing before. All calls in tests to ->getMessage() have been replaced with ->getMarkup() without any type checking, if everything passes it means core uses our constraint violation exclusively, which is good.

Finally, I added tests for the new ConstraintViolation class, testing the typecasting and the way it handles symfony's violations versus ours.

I may have some test cleanup after this one, but hopefully I haven't changed much as far as how the code works, just made it a little cleaner.

Quick fix, the extractMessage method should accept an instance of ConstraintViolationInterface, not ConstraintViolation..

Here's the workspace one I missed, plus code cleanup. This should be good for FM review.

Two minor documentation fixes

Given the difficulty in changing it later, and the problems we have been having with the validation system in general, I've come to agree that the longer term solution probably deserve a follow up. After discussing with @catch in slack, Here's a new build around #2. It includes minor doc fixes and cleanup, replace a new instance of Symfony's ConstraintValidation introduced by JSON:API, and added a basic unit test to ensure we get markup out of getMessage if we put it in.

The included interdiff is the interdiff from the patch in #2, not the latest patch.

Also bumping priority to major as this is the last issue needed to be resolved for symfony 4 support in Drupal 8.8.

Removing the CR and CR tag, as this change shouldn't require a CR
Removing the FM review tag, based on slack convo with @catch

catch [10:54 AM]
@mikelutz yeah I’m +1 for #2, we have the later patch written already so could use it when we need it, but if that’s now it feels worth going with the simpler option.

I opened this PR with the Symfony Project. Maybe we can just fix this upstream. https://github.com/symfony/symfony/pull/31083

Sorry for the long comment, but I wanted to record a symfony slack convo on the PR. with Roland Franssen. Changing the return type of ::getMessage is a BC break, we might be able to target it for Symfony 5 with a discussion, but it's going to be tough to rework for SF 4.

mikelutz [Today at 12:37 AM]
@ro0NL could I pick your brain on https://github.com/symfony/symfony/pull/31083 ? (edited)

58 replies

ro0NL [2 hours ago]
it's a BC break :slightly_smiling_face:

mikelutz [2 hours ago]
Fair enough. My main question is whether you are opposed in general to getting to a place to allow stringable objects to be used as messages, or if the problem is just the fact that the current PR expands the return type for ::getMessage() to allow a stringable object in addition to a string. I could come up with another solution that avoids the bc break and solves our problem with symfony 4, while getting on a path to just handle the stringable objects natively in 5 If that would be acceptable. If you are saying that the message must be a simple string, then we will have to just solve the problem in Drupal.

ro0NL [2 hours ago]
my main concern is we cant type it

ro0NL [2 hours ago]
e.g. `stringable` is not a PHP keyword

ro0NL [2 hours ago]
`string|object` puts weight on consumer to account for both

ro0NL [2 hours ago]
passing a stringable to the constructor might work, then cast to string in getMessage() to preserve its primitive exposed type

ro0NL [2 hours ago]
so a lazy cast.. but im not sure it helps drupal :slightly_smiling_face: and if passing a string directy is possible that's the most pure

mikelutz [2 hours ago]
We’d still have to extend and add a new method to get the original object back, or override getMessage to return the object for us, which would work in symfony 4, but I assume 5 will release with php 7.2 or 7.3 as a minimum and typehint the return type of getMessage to string, so that isn’t a long term solution.

mikelutz [1 hour ago]
What about an additional optional constructor parameter to indicate getMessage should return the original object, and typecasting the return of getMessage to string if that isn’t set?

ro0NL [1 hour ago]
i see :slightly_smiling_face: if you really need the original state from getMessage() i think it violates the concept yes (edited)

ro0NL [1 hour ago]
there's no `Message` value object for this in SF

mikelutz [1 hour ago]
I’m just brainstorming. I completely understand your concerns, I’m just trying to figure out if there’s any way to get it to do what we need without breaking your BC.

ro0NL [1 hour ago]
does drupal really need to rely on `getMessage()` here?

mikelutz [1 hour ago]
We’ve been exploring options. The template just doesn’t keep enough information to rebuild our translatable markup. We also have the issue where we could rewrite core to deal with string only messages, but we are struggling with finding a way to declare to our contrib modules that passing stringable objects into constraint violations is deprecated. (edited)

mikelutz [1 hour ago]
Which, admittedly we probably shouldn’t have been doing in Symfony 3, but we did, and here I am. :confused:

ro0NL [1 hour ago]
:slightly_smiling_face:

ro0NL [1 hour ago]
btw consider `@return` a hard contract

ro0NL [1 hour ago]
> the return type is not technically a part of the interface
so it is actually :slightly_smiling_face:

mikelutz [1 hour ago]
Fair enough.

ro0NL [1 hour ago]
i think drupal should leverage `ConstraintViolationInterface`

ro0NL [1 hour ago]
from the refs i found so far, it seems drupal already has a custom `EntityConstraintViolationListInterface`

mikelutz [1 hour ago]
I appreciate your time. I’ll reconsider our options given this conversation. Let me ask you one more question: Would you consider a Stringable contract that defines a __toString() method that could be used in symfony 5 as the message parameter?

ro0NL [1 hour ago]
i think it should be dicsussed, i understand your pov

mikelutz [1 hour ago]
We are having our DrupalCon this week, and one of our core meetings was about whether Drupal 9 (releasing June 2020) should target 4.4LTS or 5.0,

mikelutz [1 hour ago]
The general thought was 4.4.LTS was probably ideal, but we left it open. I’ve been arguing we should contribute back to Symfony more than we do as a community, if we have some ability to bring issues like this up for discussion for Symfony5, It might move us to go that route, depending of course on what other changes are made.

mikelutz [1 hour ago]
Thank you again very much for this conversation. I’m going to think on this some more, and I may bug you again if I have other ideas/questions.

ro0NL [1 hour ago]
curious.. why doesnt drupal provide the message template + parameters from the Markup object to the violation?

ro0NL [1 hour ago]
then convert back on render

ro0NL [1 hour ago]
ok. TranslatableMarkup is a value object in disguise.. `getStringTranslation` relies on a global service :neutral_face:

mikelutz [1 hour ago]
Yes, translatable markup is the problem, If it was just the template and parameters we would use it, but the translatable markup is tricky. I want to review how we managed it in light of this conversation. It’s not my particular area of focus in Drupal, I’ve just been working in general on the Symfony 4 compatibility project.

ro0NL [1 hour ago]
tough one :slightly_smiling_face:

mikelutz [1 hour ago]
Again the other problem we have to deal with is our contrib modules. We can’t just fix Drupal to work with Symfony 4 constraints, we have to somehow manage modules that might be passing an object to a constraint now, and won’t be able to in Drupal 9, but we can’t trigger a deprecation error because it’s all symfony code.

ro0NL [1 hour ago]
> , so the translation happens upon constructing the violation rather than in the twig layer, which is not ideal.
is it really a problem? given it translates upon read (edited)

ro0NL [1 hour ago]
tend to believe that's the best shortcut we can take

mikelutz [1 hour ago]
It’s another option, but we have themes and modules that override the twig translation filter and do whatever, and we have places to override display languages between creating the violation and displaying it so it’s technically a BC break on our end.

ro0NL [1 hour ago]
perhaps good for now :stuck_out_tongue: we can still debate on some message object in SF

ro0NL [1 hour ago]
but for drupal, long term, i would try to move to `message template+parameters`, thus get rid of this translation service + "options" in the markup object

ro0NL [1 hour ago]
eventually it's a compatibility issue

mikelutz [1 hour ago]
Sounds good. I want to think about it more. You may be right, I just have to figure out how to deprecate that for contrib.

mikelutz [1 hour ago]
Or wrap it in a way that does what we need with translations.

ro0NL [1 hour ago]
wrapping would be safest for you and easiest for SF

ro0NL [1 hour ago]
`\Drupal::service('string_translation')` ... in a value object .. really? :stuck_out_tongue: no offense :stuck_out_tongue:

ro0NL [1 hour ago]
^ in a serializable value object :joy:

mikelutz [41 minutes ago]
Hey, at least it’s in a accessor method we can swap out for tests.. (Just kidding, I’m not going to justify it We use \Drupal::service() far too much, but we are working on it.)

ro0NL [40 minutes ago]
either it breaks serializing, or translating :stuck_out_tongue:

mikelutz [34 minutes ago]
Thank you so much for your time tonight (this morning?) I’m going to ping some other Drupal core contributors with this conversation, and we will figure out our options. I’d like to leave the PR open for another day or two until I figure out if I can do what I need in a BC way or if I should close it and reopen it when the SF5 branch is open or of I should just close it and fix Drupal.

ro0NL [20 minutes ago]
morning still :smile:

ro0NL [20 minutes ago]
the bc break is a no-go :slightly_smiling_face: so you could propose cast on read i guess

ro0NL [20 minutes ago]
or an RFC issue to discuss e.g. a `Message` object

ro0NL [19 minutes ago]
but drupal needs to be aware of it's contract violations :slightly_smiling_face:

mikelutz [16 minutes ago]
One more stupid brainstorm thought, and I apologize because I’m sure there is policy documentation I need to find that might answer this, but since I have your ear: If the solution is Drupal needs to stop passing markup objects into the violation, and SF3 allows obejcts as the message because you can’t type hint scalers in php5, could/should we trigger a deprecation error in the constructor in SF 3.4.25 if $message is an object? I know it’s declared as a string in the docblock, so maybe it’s not appropriate, but it would force us to fix things in core and give our contrib a notice that they need to update to be ready for Drupal 9/ SF4

mikelutz [14 minutes ago]
Again, sorry if it’s a silly question, I’m just exploring options to fix our mistake in a way that preserves BC/dprecations for our contrib projects.

mikelutz [13 minutes ago]
2am here, been a long week of DrupalCon :slightly_smiling_face:

ro0NL [12 minutes ago]
> could/should we trigger a deprecation error in the constructor in SF 3.4.25 if $message is an object?
we could, not sure we should :slightly_smiling_face: this is more of a static analysis concern IMHO

ro0NL [12 minutes ago]
and https://symfony.com/doc/current/contributing/code/bc.html

ro0NL [9 minutes ago]
basically it applies everywhere, where `@param string $x` became `string $x`. Not sure we want to pick out this case specifically

mikelutz [4 minutes ago]
Thanks, I’ll review that. We based the Drupal BC policy off of Symfony’s so I’ve assumed its similar to ours. I agree you shouldn’t have to, since the param is specified in the docblock. I’m just brainstorming. Like I’ve said, we can change Drupal core, for us, it’s a matter of minimizing breaking the contrib modules when we launch Drupal 9 on SF4. I understand not wanting to add an error for one particular case. I know I sound like a broken record, but thanks so much for this convo, I’ll take it back to the other contributors, and we will figure out our options.

ro0NL [2 minutes ago]
:+1::skin-tone-2:

I don't think we should commit anything that further violates ConstraintViolationInterface. I would rather commit #36 than #38 (and I'm not a huge fan of #36)

I think we need to clean up our violation use in core to respect the interface. I think we need to figure out where we are using markup or translatable markup, and use the $messageTemplate and $parameters parameters to hold what we need to reconstruct the object. We have freedom in the parameters array to do what we wish, we might be able to include data about the original class or options if we had to.

Then I think we should convince the symfony devs to add an @trigger_error in Symfony 3 if ConstraintViolation is passed a non-string, so that any contrib that might be managing violations on their own gets a notice that they aren't compatible with Symfony 4.

While #38 does continue to violate the interface, we are already doing that, so it doesn't really make things worse. It also doesn't preclude any of the other options, up to and including going back to the symfony violation and deprecating the new one we introduce here. It would also make continuing to keep up with testing symfony 4.3 and on much easier. While I'm still convinced ultimately we should keep the symfony validator, and refactor to respect it's interfaces, I won't argue against committing #38, if that's what we want to do. It's probably the best way to keep things moving right now, and if we don't keep things moving then the symfony changes will get ahead of us again.

I will repost #38 again, but I want to Just test a few things here. First off, we seem to start off this interface violation in our instantiation of DrupalTranslator::trans() which is supposed to return a string, yet returns an object. If that returns a string like it is supposed to, then I would expect the same errors to pop up as do with SF4.

And if we generated the translated markup from the violation afterwards, I would expect them to mostly go away.

SF5 is open now, so I'll keep an eye out for when they start adding in the return typehints.

Here's a test patch of the reversion against Symfony 4.4-dev. I don't expect it to be green, there should be about 5-6 failures from other issues. If there are more, then we may need some extra tweaking in a follow-up.

Those 6 failures match my expectations and are covered under other outstanding sf 4.4 issues. All of the original errors are gone, so +1 to revert.