Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Symfony 4 uses strict types in the Constraint system, typecasting translateable markup to strings. This causes the twig system to make the string html safe, escaping the html we include in the message.
Proposed resolution
Create a subclassed ConstraintViolation that supports MarkupInterface and use that in core.
Remaining tasks
Create the subclass
Replace usage throughout core
Change Record?
User interface changes
none.
API changes
Switch to the subclassed Violation class for core violations.
Data model changes
none.
Comments
Comment #2
mikelutzLets try this...
Comment #4
catchThat looks a lot less painful than it could have been!
Comment #5
mikelutzI need to fix this one. Anything else I missed?
Comment #6
mikelutzI'm glad it was that easy, I wasn't sure it would fly like that.
Comment #7
alexpottOur pain will come in Symfony 5 when getMessage() is
getMessage(): string
and strict types is enabled.Comment #8
mikelutzre #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.Comment #9
mikelutzComment #10
Gábor Hojtsy@alexpott: do we need to use getMessage() if it does not allow us to do what we need? We can introduce our own API to wrap this one?
@mikelutz: I think the answer to that would inform where we should be going.
Comment #11
larowlan:)
Should we be using this class somewhere? Like in the validator? Feels like that is missing from the patch
Comment #12
mikelutz@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.
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.
Comment #13
larowlanYeah, ignore me - ConstraintValidationBuilder is where they're constructed, and you've updated the use statement there.
Re #7 I think if we extend the interface that give people the option of either, although with the current patch, they can always cast the return item to a string if needed, so perhaps we just leave it as is?
@alexpott thoughts?
Comment #14
alexpottI get the feeling that we should plan for the future here. It feels like we have some options.
Option 1
We should extend ConstraintViolationInterface and implement getTranslateMarkup() and we need to extend the constructor to take the TranslatableMarkup options.
The new object that implements our ConstraintViolationInterface should an an
@trigger_error()
to its getMesage() implement and say that in Drupal 9 this will throw an exception because our implementation does not support this.And in Drupal 8 core we need to convert all the usages of getMessage to getTranslateMarkup() - its implementation looks something like:
Option 2
We add a new utility class that translates a violation using the public accessors - I think plurals will prove impossible on this one.
Option 3
We extend ConstraintViolation and make it implement MarkupInterface and re-implement
__toString()
to only return the message and then remove calls to getMessage().Comment #15
alexpottOption 4
Wrap calls to getMessage() in Markup::create() with docs as to why this is correct.
Comment #16
larowlanOption 5
Ignore the constructor and extend the class to add a factory method?
Comment #17
alexpott@larowlan I don't think that that is an option because at some point we're going to run into return types with Symfony. That's why I think we have to go for one of option 1 / 2 / 4
Comment #18
mikelutzThe 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.
Comment #19
mikelutzIt 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.
Comment #20
Gábor HojtsyComment #21
mikelutzThrowing 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
Comment #23
mikelutzIt 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.
Comment #24
mikelutzYup, it got ugly, and I'm not sure this fixes all of them. :-(
Comment #26
mikelutzWell, for completion's sake, here's the last test fixed, plus some coding standards.
Comment #27
catch#26 looks OK from a release management standpoint in terms of how the deprecation is done, not the nicest change to have to make though, whether any of our other options are better is a different question.
Comment #28
mikelutzRerolling with updates for JSONAPI
Comment #30
mikelutzA couple more from revisionable taxonomy and menu links
Comment #31
mikelutzI'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.
Comment #32
mikelutzHoping 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:
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.
Comment #33
mikelutzQuick fix, the extractMessage method should accept an instance of ConstraintViolationInterface, not ConstraintViolation..
Comment #35
mikelutzHere's the workspace one I missed, plus code cleanup. This should be good for FM review.
Comment #36
mikelutzTwo minor documentation fixes
Comment #37
catchSo the only thing that bothers me here a bit is that we have the helper to do InstanceOf checks, but when we get to 9.x, we should just be able to use getMarkup() in these locations, which means we'll need to change these places again.
Having said that, the number of places that we use the helper is very low, so it's not introducing a lot of technical debt - once we require getMarkup() everywhere we can deprecate the helper message (probably for 10.x) and stop using it in core.
That makes me wonder again about the patch in #2 - which is more of a stop-gap, but doesn't require changing things around twice. Given the number of issues we've hit with the validation system generally, I'm not sure we're saving much maintenance burden by using it any more, but figuring that out isn't in scope here. If we did go for option #2 we should still open a follow-up for the more comprehensive patch here too.
Comment #38
mikelutzGiven 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
Comment #39
alexpottI guess this is okay as a stopgap. We need a CR to tell people to use the new Drupal version of the class. As stated several times - I think strict typing is going to play havoc with our Stringable objects used for markup and translations so in the long run we're going to need to push for an upstream (PHP core fix).
Comment #40
mikelutzI opened this PR with the Symfony Project. Maybe we can just fix this upstream. https://github.com/symfony/symfony/pull/31083
Comment #41
mikelutzSorry 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:
Comment #42
catchMoving back to needs review, it doesn't look like Symfony will change this for us. Also bumping to critical since this blocks Drupal 9.
Comment #43
alexpottSo we know this is eventually going to break because of return types so the question for me is given that what should our approach be. I'm not sure how we do what we need to do and satisfy \Symfony\Component\Validator\ConstraintViolationInterface
Comment #44
catchIn my view we should:
- Commit #38 in this issue since it improves the situation without making things worse, even if it's mainly pushing things off.
- Open a new issue to track the upstream Symfony issue (which will be Symfony 5 if at all https://github.com/symfony/symfony/pull/31083)
- Probably on the same follow-up, re-upload #38 so we don't forget about it.
- Also maybe a separate issue to discuss whether we're actually benefiting from using the Symfony validator component, since we're doing a lot of work to maintain our implementation of it, more than we do to maintain a fair bit of our own code without such big dependencies.
Comment #45
mikelutzI 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.
Comment #46
catchI've opened #3054535: Discuss whether to decouple from Symfony Validator to discuss the broader issues with our use of the validation component.
My preference here is still to commit #38 and then keep going in follow-ups (either with #36 or a different direction if we come up with one).
Comment #47
mikelutzWhile #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.
Comment #48
catchYes just to be clear this is why I think it's fine to commit - we're not making things worse, and we unblock more general Symfony 4/5 compatibility efforts.
Comment #49
mikelutzI 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.
Comment #51
dawehnerReading through the issue #38 seems to be a good pragmatic step forward.
Looking at #49 I'm a bit confused. I would not expect the translatable markup object to know about constraint violations. That seems to be dependencies the wrong way round. If you think about it, it's probably actually a circular dependency, even if its loose.
Comment #52
jibranI agree with #51. The patch #38 seems to be the way forward. I don't think we can fork the Symfony Validator component.
Comment #53
catchRe-uploading #38.
Comment #54
jibranThis is ready so setting it RTBC. Added a change record https://www.drupal.org/node/3059344. Feel free to update it with more info.
Comment #55
jibranComment #56
alexpottLet's do this for now to move on. We know this area is going to bite us again but hopefully we can move forward in the other issues and get to a better solution at some point.
Committed 20496dd and pushed to 8.8.x. Thanks!
Comment #58
alexpottI don't know if we need release notes here. It feels very in the weeds.
Comment #59
catchSeems noisy for release notes to me as well.
Comment #60
mikelutzSF5 is open now, so I'll keep an eye out for when they start adding in the return typehints.
Comment #62
Pasqualle(comment based on Slack communication)
Please reopen the issue. The problem was fixed in Symfony:
https://github.com/symfony/symfony/commit/5ce3a61660862cb22c8c4ca826543e...
The commit should be reverted when Mike confirms he is ready to test against Symfony 4.4 head, but before Drupal 8.8 is released.
Comment #63
mikelutzHere'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.
Comment #65
mikelutzThose 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.
Comment #67
larowlanReverted this after confirming with @mikelutz
Dropping priority and marking as fixed, upstream
Comment #68
Wim LeersMarked https://www.drupal.org/node/3059344 as a draft again, to match the reverted status of this.