Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

Status: Active » Needs review
FileSize
10.19 KB

Patch created with:

find . -path './.git' -prune -o -not \( -name '*.js' \) -type f -exec sed -i -e "s/Symfony\\\Component\\\Validator\\\ExecutionContextInterface/Symfony\\\Component\\\Validator\\\Context\\\ExecutionContextInterface/g" {} \;

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.

klausi’s picture

Issue tags: +find and replace

Tagging, so that I can find my useful "find" commands later.

dawehner’s picture

+1

Status: Needs review » Needs work

The last submitted patch, 2: execution-context-3-2721179-2.patch, failed testing.

jibran’s picture

Status: Needs work » Closed (duplicate)
catch’s picture

Status: Closed (duplicate) » Needs work

Re-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.

catch’s picture

catch’s picture

Version: 8.2.x-dev » 8.3.x-dev

@stof on that issue said use the ConstraintValidator base class, which we don't. Patch incoming.

catch’s picture

Status: Needs work » Needs review
FileSize
11.66 KB
4.28 KB

Two of our constraint validators extend Constraint, didn't touch those, but here's the rest (untested).

dawehner’s picture

Interesting

Status: Needs review » Needs work

The last submitted patch, 10: 2721179-9.patch, failed testing.

klausi’s picture

So again we find out that this does not work for us :(

Should we close this again?

catch’s picture

@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.

catch’s picture

Status: Needs work » Needs review
Issue tags: +rc target
FileSize
13.87 KB
11.33 KB

So 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.

catch’s picture

Priority: Normal » Major

Bumping 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.

dawehner’s picture

I love that plan in general

klausi’s picture

Status: Needs review » Needs work
Issue tags: +Dublin2016

OK, 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:

  1. +++ b/core/lib/Drupal/Core/Validation/ConstraintValidatorBase.php
    @@ -0,0 +1,31 @@
    +use Symfony\Component\Validator\Violation\ConstraintViolationBuilderInterface;
    +use Symfony\Component\Validator\Violation\LegacyConstraintViolationBuilder;
    

    unused use statements.

  2. +++ b/core/lib/Drupal/Core/Validation/ConstraintValidatorBase.php
    @@ -0,0 +1,31 @@
    + * Copied from \Symfony\Component\Validator\ConstraintValidator with minor
    + * modifications.
    

    what are those modifications? I think we use a different ExecutionContextInterface on the initialize method, right?

  3. +++ b/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php
    @@ -2,9 +2,11 @@
    +use Drupal\Core\Validation\ConstraintValidatorBase;
    +
    +use Symfony\Component\Validator\ConstraintValidator;
    

    that empty line should be removed?

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
@@ -2,10 +2,11 @@
+use Symfony\Component\Validator\ConstraintValidator;
+use Symfony\Component\Validator\Context\ExecutionContextInterface;

unused use statements.

catch’s picture

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.

Well 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.

what are those modifications? I think we use a different ExecutionContextInterface on the initialize method, right?

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.

klausi’s picture

OK, cleared up the nitpicks and removed unused stuff.

klausi’s picture

FileSize
13.18 KB
523 bytes

Forgot removing one empty line.

catch’s picture

Clean-up looks good to me.

alexpott’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +8.2.0 release notes
FileSize
15.73 KB
8.53 KB

Here'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.

alexpott’s picture

catch’s picture

I 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).

catch’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: -rc target
FileSize
21.29 KB

That works, which means we don't need the new base class added here, which means this is no longer 8.2.x sensitive.

catch’s picture

FileSize
16.79 KB
effulgentsia’s picture

Version: 8.3.x-dev » 8.2.x-dev
Issue tags: +rc target triage
  1. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php
    @@ -6,18 +6,18 @@
       /**
        * Stores the validator's state during validation.
        *
    -   * @var \Symfony\Component\Validator\ExecutionContextInterface
    +   * @var \Symfony\Component\Validator\Context\ExecutionContextInterface
        */
       protected $context;
    

    Here, and in similar classes: now that we are extending the base class, do we need this definition at all?

  2. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php
    @@ -6,18 +6,18 @@
    +use Symfony\Component\Validator\Context\ExecutionContextInterface;
    

    In which case, we don't need this either.

this is no longer 8.2.x sensitive

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:

  • Sites will benefit from all the S3 awesomeness for the next 6 months, or
  • We'll learn about other incompatibilities between S3 and D8 from people running real world sites, not only from people testing 8.3 while it's in development.

Meanwhile, #26 looks like low disruption to me, so requesting it be considered per https://www.drupal.org/core/d8-allowed-changes#rc.

catch’s picture

If 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.

catch’s picture

Status: Needs review » Needs work

#1 and #2 from 28 are both correct too.

effulgentsia’s picture

Version: 8.2.x-dev » 8.3.x-dev
Issue tags: -rc target triage

Discussed 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.

catch’s picture

Status: Needs work » Needs review
FileSize
20.52 KB
4.73 KB

Updating for #28 and #29.

klausi’s picture

Status: Needs review » Needs work
Issue tags: -8.2.0 release notes +Needs change record

I agree that having the constraint and the validator split up as Symfony does is probably a better idea where we avoid a Drupalism.

  1. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php
    @@ -4,31 +4,24 @@
    +use Symfony\Component\Validator\Context\ExecutionContextInterface;
    

    unused use statement.

  2. +++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidator.php
    @@ -4,31 +4,24 @@
       /**
        * Stores the validator's state during validation.
        *
    -   * @var \Symfony\Component\Validator\ExecutionContextInterface
    +   * @var \Symfony\Component\Validator\Context\ExecutionContextInterface
        */
       protected $context;
    

    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.

catch’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
20.29 KB

Think I got them all this time. Also added a change record.

catch’s picture

Right interdiff, wrong patch :(

The last submitted patch, 34: 2721179-34.patch, failed testing.

klausi’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Patch looks good, change record looks good.

alexpott’s picture

Issue tags: +8.3.0 release notes

Committed 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++

  • alexpott committed 92a19bd on 8.3.x
    Issue #2721179 by catch, klausi, alexpott, dawehner, effulgentsia:...
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.