Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
ExecutionContextInterface is deprecated and has been moved in Symfony 3.
Proposed resolution
Update our usage to prepare the Symfony 3 update.
Remaining tasks
Patch
User interface changes
none.
API changes
none.
Data model changes
none.
Comment | File | Size | Author |
---|---|---|---|
#35 | 2721179-35.patch | 20.04 KB | catch |
#34 | 2721179-34.patch | 20.29 KB | catch |
#34 | interdiff.txt | 2.17 KB | catch |
#32 | interdiff.txt | 4.73 KB | catch |
#32 | 2721179-32.patch | 20.52 KB | catch |
Comments
Comment #2
klausiPatch created with:
Unfortunately we cannot make this change before upgrading to Symfony 3 because ConstraintValidatorInterface is different between Symfony 2 and 3. So we will have to roll this directly into #2712647: Update Symfony components to ~3.2. So the massive test fails this will trigger on Symfony 2 are expected.
Still posting the patch here to have this as standalone issue.
Comment #3
klausiTagging, so that I can find my useful "find" commands later.
Comment #4
dawehner+1
Comment #6
jibranDuplicate of #2712647: Update Symfony components to ~3.2.
Comment #7
catchRe-opening, we should open an upstream Symfony issue to at least discuss the hard bc break.
To summarize again, the issue is that ConstraintValidatorInterface in 2.8 and 3.0 is identical except that the use statement for ExecutionContextInterface has been changed to point to the new interface.
What this means is that if you have a class implementing ConstraintValidatorInterface, it's impossible to update that class to be compatible with the new ExecutionContextInterface. What it would probably need is a new version of ConstraintValidatorInterface which type hints the new ExecutionContextInterface in 2.8.
Comment #8
catchHere's that issue: https://github.com/symfony/symfony/issues/20088
Comment #9
catch@stof on that issue said use the ConstraintValidator base class, which we don't. Patch incoming.
Comment #10
catchTwo of our constraint validators extend Constraint, didn't touch those, but here's the rest (untested).
Comment #11
dawehnerInteresting
Comment #13
klausiSo again we find out that this does not work for us :(
Should we close this again?
Comment #14
catch@klausi the test fails are mostly from UserMailRequired and LinkTypeConstraint - I didn't convert these classes because they extend from Constraint, which means they can't also extend from ConstraintValidator, and because I only had 5 minutes to do this patch before a phone call.
However we should be able to add a trait that matches the methods on Constraint so that they can extend from ConstraintValidator instead.
Comment #15
catchSo that was mildly interesting:
The validate method on a constraint validator, type hints on Constraint.
Those two classes both inherit from Constraint (making them Constraints), and implement ConstraintValidatorInterface. This means you can't make a trait doing the same as Constraint, because it's then not a Constraint.
So Symfony's base class is not that useful for this case, but I made a new base class, that inherits from Constraint and implements ConstraintValidatorInterface which we can use for the LinkItem and UserMailRequired constraints. This appears to work.
This means Drupal core will have to update a use statement, but any contrib constraint validators that are also constraints would be able to use our base class and not have changes when we update to 3.1
Didn't do a full test run, but a couple of test passed with this.
Comment #16
catchBumping to major.
Also asked on the upstream issue whether they'd accept such a base class - if 2.8 had one, then it'd mean no bc break at all. For me this is a good enough workaround for us though. What we'd have to do:
1. Change notice to tell people to use Symfony's base class and our base class instead of implementing ConstraintValidatorInterface once this patch lands. Could possibly squeeze it into 8.2.x so it's available sooner than later.
2. In 8.3.x or 8.3.x, the Symfony 3.1 update is then the composer.json change, plus a one line change to the new base class to update the new use statement.
As long as people update for #1 in advance of that, #2 will be non-breaking for contrib.
Comment #17
dawehnerI love that plan in general
Comment #18
klausiOK, this makes a bit of sense. So after this patch we just have to update one use statement in ConstraintValidatorBase.php when we update to Symfony 3.
The problem is for contrib that this new base class does not exist in older versions of Drupal. So they have exactly the same problem and work to do as when we break compatibility. They need to announce that their most recent code only works with the most recent version of Drupal. If we put this into 8.2.0 and do the Symfony 3 update in 8.3.0 then at least modules can make them compatible with those 2 versions, but they will immediately be broken on 8.1.x. Do we consider this better than nothing?
Random small nitpicks:
unused use statements.
what are those modifications? I think we use a different ExecutionContextInterface on the initialize method, right?
that empty line should be removed?
unused use statements.
Comment #19
catchWell not quite, the timing is staggered. If we add this to 8.2, they can update for it any time between then and us updating to Symfony 3. 8.1.x support gets dropped October 5th and if you're updating contrib projects after that date but not updating core then you're on your own.
If we get this into 8.2.x and update to Symfony 3 in 8.4.x, then the window will be even longer.
I think we can drop this comment. The Symfony base class implements a load of 2.5-compatible methods, I dropped all those. It's really just a class with one property and one one-line method which isn't worth mentioning, we can summarize this issue a bit instead.
Comment #20
klausiOK, cleared up the nitpicks and removed unused stuff.
Comment #21
klausiForgot removing one empty line.
Comment #22
catchClean-up looks good to me.
Comment #23
alexpottHere's a patch with a coding standards cleanup and a convert of use class strings to using classname::class - just makes things simpler if we need to change this again (unlikely but it is better).
Reviewing the patch - this is the best compromise between insulating Drupal core and custom from the upgrade to Symfony 3. It is likely we'll uncover more pain points for contrib once the latest branch adopts Symfony 3 because that's what is used for contrib testing.
I think doing this in 8.2.0 makes sense because the change is minimal and has a very low chance of being a breaking change and gives the maximum possible time for contrib to update to ensure they don't break when we move to Symfony 3.
Comment #24
alexpottComment #25
catchI have a patch on #2810367: Stop mixing Constraints and ConstraintValidators that doesn't require the base class for core. If we think that's a better solution, we could move the patch to here and do this change in 8.3.x - it'd mean contrib needs to update to use that pattern as opposed to updating the new base class (more change, but no dependency on a new API in core).
Comment #26
catchThat works, which means we don't need the new base class added here, which means this is no longer 8.2.x sensitive.
Comment #27
catchComment #28
effulgentsia CreditAttribution: effulgentsia at Acquia commentedHere, and in similar classes: now that we are extending the base class, do we need this definition at all?
In which case, we don't need this either.
While it's great that this is less 8.2.x sensitive than #23 and earlier, I still think we should get it into 8.2, so tagging for rc target triage. If we get this in, then as far as we know, contrib modules will be able to have tagged releases that both require Symfony 3.0 (or 3.1 or 3.2) and can work on a Drupal 8.2 site. And that's high impact because it will mean either:
Meanwhile, #26 looks like low disruption to me, so requesting it be considered per https://www.drupal.org/core/d8-allowed-changes#rc.
Comment #29
catchIf we put the validators in the plugin namespace we can rely on Symfony's magic naming support for validators, would save a few more lines so will do that in next update.
Comment #30
catch#1 and #2 from 28 are both correct too.
Comment #31
effulgentsia CreditAttribution: effulgentsia at Acquia commentedDiscussed with @xjm, @catch, @alexpott, and @cilefen, and we decided that even though this patch looks to be low risk, that we're not comfortable rushing it into 8.2.0 so close to its release.
People who want to experiment with running a D8.2 site on Symfony 3 can do so by applying the patch themselves. It's quite possible that in the course of updating Drupal 8.3 to Symfony 3, that we'll learn of other changes that are needed anyway, so such sites may end up needing to apply several patches.
Comment #32
catchUpdating for #28 and #29.
Comment #33
klausiI agree that having the constraint and the validator split up as Symfony does is probably a better idea where we avoid a Drupalism.
unused use statement.
this variable already exists on the parent and can be removed. Also elsewhere.
Then we should have a change record that tells Drupal contrib authors to split up their constraints and validators in the same way and extend ConstraintValidator.
Comment #34
catchThink I got them all this time. Also added a change record.
Comment #35
catchRight interdiff, wrong patch :(
Comment #37
klausiPatch looks good, change record looks good.
Comment #38
alexpottCommitted 92a19bd and pushed to 8.3.x. Thanks!
We need to mention this one in the release notes of 8.3.0. It's great that we found this way of resolving this problem. @catch++
Comment #40
alexpott