Update to Symfony 2.5

There's no cons made in #2161397: Update to Symfony 2.4.1 about stable version, so just following and +1 to rtbc

Likely needs a reroll after #2276119: Upgrade Guzzle to version 4.1.0

@andyceo
I guess we want to avoid to update all external dependencies ... let's try to just update the symfony components.

Based upon my last review.

Wow symfony 2.5 is indeed not 100% BC compatible.

This really naive approach lead to not working saving of entities.

Added an issue on https://github.com/symfony/symfony/issues/11393#issuecomment-49033154 but i basically have no fucking clue.

working on it.

It turns out that there is a BC layer we can use, so I opened a follow up to adapt our code: #2307687: Update the metadata-typedata-validation code to use the 2.5 api

Okay, we need to be compatible with 2.4 instead.

It seems to be that the TODO is automatically resolved with this update.

.

+++ b/core/modules/user/src/Tests/UserValidationTest.php
@@ -104,15 +104,9 @@ function testValidation() {
-    //   https://drupal.org/node/2023465.

https://www.drupal.org/node/2023465 is still open
Other then this RTBC.

https://www.drupal.org/node/2023465 is still open
Other then this RTBC.

The other one can just be marked as fixed if this one is in, so RTBC.

I would love to have a review from someone who actually understands the validator.

.

1. +++ b/core/modules/user/src/Tests/UserValidationTest.php
   @@ -104,15 +104,9 @@ function testValidation() {
   -    // @todo There are two violations because EmailItem::getConstraints()
   -    //   overlaps with the implicit constraint of the 'email' property type used
   -    //   in EmailItem::propertyDefinitions(). Resolve this in
   -    //   https://drupal.org/node/2023465.
   -    $this->assertEqual(count($violations), 2, 'Violations found when email is too long');
   +    $this->assertEqual(count($violations), 1, 'Violations found when email is too long');
        $this->assertEqual($violations[0]->getPropertyPath(), 'mail.0.value');
        $this->assertEqual($violations[0]->getMessage(), t('%name: the email address can not be longer than @max characters.', array('%name' => $user->get('mail')->getFieldDefinition()->getLabel(), '@max' => EMAIL_MAX_LENGTH)));
   -    $this->assertEqual($violations[1]->getPropertyPath(), 'mail.0.value');
   -    $this->assertEqual($violations[1]->getMessage(), t('This value is not a valid email address.'));
   

   These changes I have no understanding of :( Who can RTBC this part?

2. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -15,8 +15,8 @@
   -use Symfony\Component\Validator\ValidatorInterface;
    use Symfony\Component\Validator\Validation;
   +use Symfony\Component\Validator\Validator\ValidatorInterface;
   
   @@ -298,7 +298,7 @@ public function getPropertyInstance(TypedDataInterface $object, $property_name,
   -   * @param \Symfony\Component\Validator\ValidatorInterface $validator
   +   * @param \Symfony\Component\Validator\Validator\ValidatorInterface $validator
   
   @@ -308,7 +308,7 @@ public function setValidator(ValidatorInterface $validator) {
   -   * @return \Symfony\Component\Validator\ValidatorInterface
   +   * @return \Symfony\Component\Validator\Validator\ValidatorInterface
   

   This change is not needed until Symfony 3.0, but its fine to maintain parity with 2.5.

3. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -316,6 +316,7 @@ public function getValidator() {
   +        ->setApiVersion(Validation::API_VERSION_2_4)
   

   That is a neat trick.

I have pinged @fago on twitter for #29.1. https://twitter.com/JibranIjaz/status/494463802180530178

thanks for pinging me.

These changes I have no understanding of :( Who can RTBC this part?

Looking through this, I was concerned that a violation fail suddenly disappeared. Having a closer look, it turns out that symfony changed e-mail validation: It now either works in strict RDF compliant variant based on https://github.com/egulias/EmailValidator (off by default) or it's based on a simple regex, which does not cover the length check any more as before.

See https://github.com/symfony/symfony/issues/1581 - reading the issue I was not able to determine the reason for moving away from filter_var() :(

So we have three options:
- Go with strict email validation, RFC compliant but slower
- Stay with the simple regex
- Write our own email constraint implemenation staying with filter_var().

Drupal 7 used to validate emails with filter_var(), so migrating to a simpler regex seems not feasible to me. But when we go with strict validation, what happens to some pre-existing mail adresses failing validation after upgrading?

We validate email addresses rarely enough that I am not concerned about performance.

Given that the RFC is ridiculously permissive, *are* there email addresses that wouldn't validate but would validate with a simple regex? Normally the problem of not using filter_var() or RFC is that you have lots of false negatives, not false positives.

Also please keep in mind that there email in international punycode that better to validate properly

Opened a follow up to bring the external library into core because dries has to decide whether this library should be included: #2313669: Bring in egulias/EmailValidator for RFC compliant email address validation

In general I would also favour standard-compliance over performance improvements for some usecases. I am not sure
but I would argue that people with maximum needed import speed want to validate before or after doing the saving step and there might be a way to disable validation
temporarily for those cases

Let's add a @todo with above issue so that we can RTBC this.

Here is what I posted in #2313669: Bring in egulias/EmailValidator for RFC compliant email address validation: Ideally, this would be fixed in PHP but sadly, that is not a realistic expectation. It seems like a lot of extra code but in general, I prefer to add a library rather than having to subclass Symfony to change its behavior. I'm not thrilled with it but all things considered, I'm ok with adding this library.

http://symfony.com/blog/symfony-2-5-3-released

Symfony 2.5.3 has just been released. It fixes a lot of issues regarding the backward compatibility of the Validator component. That should ease the migration from 2.4. If you find any other problems when upgrading from 2.4 to 2.5, please consider opening an issue.

2.5.3 patch

Now with core changes.

Let's be honest this is a critical issue.

This needs to have composer update run again because today Symfony released several security updates, including http://symfony.com/blog/cve-2014-5244-denial-of-service-with-a-malicious..., which addresses the same host header vulnerability in https://www.drupal.org/SA-CORE-2014-003.

And another "get on the latest thing".

A rerolled with the following procedure:

1. I branched and applied the patch.
2. I reset composer.lock and the core/vendor directory
3. I rebased 8.0.x fixing one conflict in UserValidationTest
4. Then I ran composer update.

Oops - I forgot to include new files in core/vendor and composer.lock.

This is including changes to all vendor libraries (look at core/vendor/doctrine, for example).
It should only update Symfony.

@cilefen
https://www.drupal.org/node/2278353#comment-8968689 describes how to update just the ones we want there.

As described. Should we not specify exact versions in composer.json?

composer update symfony/class-loader symfony/css-selector symfony/dependency-injection symfony/event-dispatcher symfony/http-foundation symfony/http-kernel symfony/routing symfony/serializer symfony/validator symfony/yaml symfony/debug

There is also symfony/process symfony/property-access symfony/translation. Should those be included? property-access is removed if you update it.

This works: composer update symfony/*.

This patch includes upgrades to symfony/property-access (removed), symfony/process, and symfony/translation.

composer update symfony/*
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Removing symfony/property-access (v2.4.1)
  - Removing symfony/process (v2.3.4)
  - Installing symfony/process (v2.5.4)
    Loading from cache

  - Removing symfony/translation (v2.3.4)
  - Installing symfony/translation (v2.5.4)
    Loading from cache

I just noticed that we missed YAML update. The composer.json had it specified as '2.5' instead of '2.5.*', which caused it to load the version 2.5.0. I am attaching the patch and interdiff with it changed to 2.5.* and consequently, symfony/yaml 2.5.4.

Please note that for simplicity's sake, I did not start with the patch in #57. I just modified the composer.json file and ran composer update symfony/*. I applied the patch later for interdiff purposes.

Ah, the process failed on me. I didn't realize there were changes in Drupal side of things. Here is the proper patch. The interdiff in #58 is still the correct one.

Should we be doing this? Please look at the Symfony release cycle if you are not already familiar with it.

If we upgrade to every Symfony release, we will endure this process about every 8 months, forever. 2.6 is due to be released in November and one could argue we should postpone this issue until then. No one has raised any reason for upgrading to 2.5 except for the fact that 2.5 exists.

I am making the case for staying on Long Term Support versions of Symfony. That would mean staying on 2.3 until mid-2015 when 2.7 is released. By staying on 2.3 we would not, for example, need to bring in another library to cover a problem in 2.5. And maybe we can influence getting the validator sorted out on the Symfony side before 2.7 lands.

It is a philosophy question: Should Drupal use foundational library version with long-term-support (currently 2.3) or the latest features (2.5, which is EOL in November 2014)?

Sorry, I was wrong on dates:

• 2.3 End of Support is May 2016
• 2.5 End of Support is July 2015

This doesn't change the core of my argument.

Given that we are already relying on features in post-2.3 Symfony releases (the RequestStack, in particular) I think tracking stables is a done deal at this point.

A third of the problems we have with this process are due to our misuse of composer. Fix that and those issues go away. For the rest, we can put wrappers around the parts that are known to be problematic to abstract them from the systems that care.

I think we could also decide to stop tracking stables once 2.7 comes out in 8 months, and stick on that.

I agree with Crell on this. We are already using 2.4 and we should go with the updates. We may or may not decide to update to 2.6 depending on the API breaks and release state of Drupal at that point. If Drupal has entered beta by the time 2.6 is out (Nov 2014), then we only upgrade if we don't break API or can write wrappers around it. This is a likely scenario.

However, given that 2.7 is LTS, we should plan an upgrade and if there are API breaks, we should consider writing wrappers. Drupal 8 would probably be in final release by the time 2.7 is out and it would be much difficult to deal with API breaks then, considering we are using semantic versioning now.

I believe catch has previously stated his intent is to track stables until Drupal 8 gets to an LTS release, then freeze on whatever it's on at the time. He may have reconsidered that position but that's what I recall him saying in public last.

I am all for tracking stables, especially since we are in pre-beta. I am just concerned how do we handle an upgrade to 2.7 if there are API breaks. It breaks the promise of semantic versioning.

Alternatively, we should make it clear that Drupal would be API compatible but if someone is using it's dependencies (Symfony) directly, they may break API compatibility. We should revisit this when Drupal gets close to a release candidate.

#67 still covers my position. We need to be able to submit upstream bug/performance/docs fixes, and potentially features, to Symfony, and running 2.3 would mean also trying to get those backported through several different branches, or subclassing/forking components ourselves if/when that doesn't happen. All of that is a lot more painful than upgrading every few months, especially at the moment when we don't have a stable release out ourselves.

Additionally, being able to take advantage of 2.4+ changes in Symfony should mean less work to do in Drupal 9 when that opens, for both core and contrib modules.

Everyone—thank you for the comments. It is good to have these discussions.

I updated the issue with the current consensus. Should we commit this as-is or wait for #2313669: Bring in egulias/EmailValidator for RFC compliant email address validation and the following issue which would actually integrate egulias/EmailValidator to be committed first?

@cilefen: Is there any reason this should wait. I reviewed the patch and that issue and I think either one can go in first. Sure, the other one might need a reroll. Other than that, am I missing something here?

@hussainweb As far as I am concerned, this is ready to go. We need a follow-up to #2313669: Bring in egulias/EmailValidator for RFC compliant email address validation to implement the new validator.

Reroll

+++ b/core/vendor/symfony/validator/Symfony/Component/Validator/Constraints/EmailValidator.php
@@ -36,12 +53,23 @@ public function validate($value, Constraint $constraint)
+        if ($constraint->strict && class_exists('\Egulias\EmailValidator\EmailValidator')) {
+            $strictValidator = new StrictEmailValidator();
+            $valid = $strictValidator->isValid($value, false, true);
+        } elseif ($constraint->strict === true) {
+            throw new \RuntimeException('Strict email validation requires egulias/email-validator');
+        } else {
+            $valid = preg_match('/.+\@.+\..+/', $value);
+        }

This is interesting regarding email validation.

This is interesting regarding email validation.

Well yes, this is the point. Symfony either uses a trivial validation or a 100% proper one using the external package, which is why I filled the additional issue.

@dawehner @berdir and I discussed the email validation question on IRC and concluded that since valid_email_address() still exists and uses filter_var() then it makes no sense to have the validator class act differently. Because of that, I propose we add a new validator that overrides Symfony's that does filter_var().

#2313669: Bring in egulias/EmailValidator for RFC compliant email address validation will be non-critical.

This is a try at a filter_var() validator.

Now that #80 is green, and if what I did is acceptable, we could stay with filter_var() and upgrade to Symfony 2.5 now. Then we could go on to #2313669: Bring in egulias/EmailValidator for RFC compliant email address validation as an enhancement.

@cilefen
For such issues it helps dramatically if you post an additional patch which just shows the changes in composer.json and core code and leaves out the vendor code changes.

This looks like a perfect way to solve this issue for now!

This is the changes in #80 for core/lib and composer.json only.

#2313669: Bring in egulias/EmailValidator for RFC compliant email address validation is in so we should go with the new library. This patch attempts to use the new validator library in "strict" mode. I am not sure if I implemented it properly in the ConstraintManager.

Oops.

I accidentally updated all the vendor libraries. It should be composer update symfony/*.

Symfony 2.5.5 just released.

I can't believe I did it again. The prior patch contains every upgrade, which we don't want. This contains symfony only. Say it with me! composer update symfony/* Now I feel better.

+1 rtbc this blocks a security team follow-up

Let's get caught up.

Talked to effulgentsia about this patch and he felt it's probably best for catch to look at this one and decide whether to try and get it in before/after beta.

Sounds like Symfony 2.6 has some new shiny things:

http://symfony.com/blog/new-in-symfony-2-6-support-for-object-maps-in-ya...
http://symfony.com/blog/new-in-symfony-2-6-date-support-for-validator-co...
http://symfony.com/blog/new-in-symfony-2-6-lockhandler

And will probably release in November: http://symfony.com/roadmap?version=2.6

Here is an incomplete list of new additions : http://symfony.com/blog/symfony-2-6-fast-approaching-its-stabilization-p...

Committed/pushed to 8.0.x, thanks!

I think that Drupal 8.x should upgrade Symfony in the future and stay in Symfony 2.7, that will be released in May 2015.
Symfony 2.7 will be a long term support version with security fixes until May 2019.

This will reduce the work needed to mantain Symfony in Drupal 8.x LTS version.

What you think?

There is certainly a problem with the overlap of the potential commit time of symfony 2.7 and Drupal 8. We have to think about that now,
as it might be a huge risk to switch to 2.7 during getting the release out.

@int could you please file a new issue to discuss that?

@dawehner: It is certainly risky but I am not sure if it is a huge risk. Symfony 2.3 onwards use semver, so there will be a backward compatibility layer. We had a layer here too but we couldn't use that because of email validation concerns.

Of course, we can't say anything for sure right now but we don't have to rule it out just yet.

@int: See #67 and #70.

I'm sorry that the changes in the Symfony 2.5 API seemed to have caused troubles. Is there still anything I can do to help?

Feel free to ping me whenever you have problems with the Validator component.

@webmozart Open new issues about the problems and mention them in a comment on this issue.

@webmozart, Oh I think I misunderstood. Yes, we have an issue to improve the validation here: #2343035: Upgrade validator integration for Symfony versions 2.5+

Followup: #2375339: Update DependencyInjection YamlFileLoader for Symfony 2.6