Problem/Motivation

#2712647: Update Symfony components to ~3.2, #2871253: Update Symfony components to 3.2.8 and #2874909: Update Symfony components to 3.3.* updated the Symfony components to 3.3.* but with 3.4.0 due out in November 2017 we should make sure Drupal 8.4.0 can ship with this.

Proposed resolution

Update composer.json once 3.4.0 has been tagged.

Remaining tasks

We need to resolve https://github.com/symfony/symfony/issues/25786 upstream.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#76 2927746-76.patch87.09 KBalexpott
#76 73-76-interdiff.txt3.38 KBalexpott
#73 2927746-73.patch83.7 KBalexpott
#73 72-73-interdiff.txt891 bytesalexpott
#72 2927746-72.patch84.58 KBalexpott
#72 70-72-interdiff.txt861 bytesalexpott
#70 2927746-69.patch83.73 KBalexpott
#70 65-69-interdiff.txt1.63 KBalexpott
#65 2927746-65.patch82.53 KBalexpott
#65 63-65-interdiff.txt2.22 KBalexpott
#65 2927746-65.patch82.53 KBalexpott
#64 2927746-64.patch82.87 KBcatch
#64 interdiff-60-64.txt2.06 KBcatch
#64 2927746-62.patch82.26 KBcatch
#64 2927746-60-62-interdiff.txt1.28 KBcatch
#60 2927746-58.patch81.28 KBalexpott
#60 51-58-interdiff.txt2.79 KBalexpott
#51 2927746-51.patch81.41 KBalexpott
#51 49-51-interdiff.txt6.21 KBalexpott
#49 2927746-49.patch79.29 KBalexpott
#49 48-49-interdiff.txt1.07 KBalexpott
#48 2927746-48.patch78.35 KBalexpott
#48 44-48-interdiff.txt8.97 KBalexpott
#44 2927746-44.patch77.02 KBcatch
#44 interdiff-2927746-43.txt2.74 KBcatch
#44 interdiff-2927746-43.txt2.74 KBcatch
#44 2927746-43.patch76.31 KBcatch
#41 2927746-41.patch73.57 KBalexpott
#41 39-41-interdiff.txt7.13 KBalexpott
#39 2927746-39.patch69.62 KBalexpott
#39 37-39-interdiff.txt4.52 KBalexpott
#37 2927746-37.patch69.3 KBalexpott
#37 35-37-interdiff.txt1.17 KBalexpott
#35 2927746-35.patch68.12 KBalexpott
#35 16-35-interdiff.txt2.59 KBalexpott
#16 2927746-16.patch66.59 KBmpdonadio
#15 2927746-15.patch146.09 KBRoSk0
#11 2927746-11.patch65.69 KBmartin107
#4 interdiff.txt2.48 KBslasher13
#4 update_symfony-34-4.patch65.68 KBslasher13
interdiff.txt41.37 KBslasher13
update_symfony-34.patch65.68 KBslasher13
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

slasher13 created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, update_symfony-34.patch, failed testing. View results

slasher13’s picture

Status: Needs work » Needs review
FileSize
65.68 KB
2.48 KB

require symfony/debug ~3.4.0

dawehner’s picture

@slasher13
I'm curious, do you think we should update to 3.4 directly, or should we execute the 3.3 update first and then deal with the additional deprecations for 3.4?

Status: Needs review » Needs work

The last submitted patch, 4: update_symfony-34-4.patch, failed testing. View results

slasher13’s picture

First we should analyse what's happening here. Many problems like:
- Fatal Error Unable to allocate shared memory segment of 1048576000 bytes: shmget: File exists (17)
- Fatal Error Insufficient shared memory!

https://dispatcher.drupalci.org/job/drupal_patches/39306/console

@dawehner: I don't know how big the influence on drush and contrib modules is. The biggest problem for contrib modules might be the handling of deprecation messages.

jonathanshaw’s picture

Issue summary: View changes
slasher13’s picture

martin107’s picture

@slahser13 thanks for posting that .. a good observation.

A minor point the bottom link is broken ... it took me a moment to realize the link works if you remove the colon!

https://symfony.com/blog/symfony-3-4-curated-new-features

martin107’s picture

Needed reroll.. I won't retest.

moshe weitzman’s picture

Can someone make sure that symfony/yaml can parse our own default services.yml. I am seeing that it can't over in the Drush queue https://github.com/drush-ops/drush/issues/3254

slasher13’s picture

moshe weitzman’s picture

Thanks for the quick reply. We should consider patching 8.4 so that fewer people see fatal errors when they upgrade to 8.5. Just a minor concern for another issue.. I just reread your message - services.yml is in progress in another issue.

mpdonadio’s picture

Looks like #15 picked up the PHP7+ versions. Took #15, stripped out the hunks for the lockfile, ran `composer update 'symfony/*' --with-dependencies` using PHP 5.5.38 from MAMP. Let's see what happens now.

The last submitted patch, 15: 2927746-15.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 16: 2927746-16.patch, failed testing. View results

cilefen credited Mile23.

cilefen credited alexpott.

cilefen credited catch.

cilefen credited jibran.

cilefen’s picture

I am checking for or adding credit for the following people who contributed substantially to the 3.3 issue that I just now marked as a duplicate:

  • slasher13
  • jibran
  • mpdonadio
  • pritish.kumar
  • deviantintegral
  • Jo Fitzgerald
  • Mile23
  • catch
  • alexpott

Thank you everyone for the continued efforts. Getting on Symfony 3.4 components will be a good thing.

alexpott’s picture

So we've got a couple of issues.
1. The yaml in sites/settings/default.services.yml is not valid :( specifically the multi-line declaration of factory.keyvalue and factory.keyvalue.expirable. This is pretty nasty because people will have copied this file. Imo we need to file an upstream issue to try to fix the hard API break.
2. Even once the yaml issue is resolved I have a problem installing Drupal. Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 20480 bytes) in core/lib/Drupal/Component/DependencyInjection/Dumper/OptimizedPhpArrayDumper.php on line 315. This suggests we have some recursion occurring in OptimizedPhpArrayDumper - something to do with the container changes.

pounard’s picture

I noticed, by (yes, this is insane, but working actually) adding the Syfmony 3.4 fullstack into Drupal (and making bundles working in Drupal 8), that Symfony's FrameworkBundle services dependencies caused Drupal's OptimizedPhpArrayDumper to fall into infinite loops. There's a lot of work to fix that, I did try, but that's not an easy one.

alexpott’s picture

The yaml issues are caused by [d594038ad9228d6a71173dab12a94b8d24de4df9] do not eagerly filter comment lines / https://github.com/symfony/symfony/pull/25241 which is part of 3.3 and 3.4. I'll open a PR to fix this as it is a regression that will make upgrading to a version of Drupal with Symfony 3.3 or 3.4 very painful.

alexpott’s picture

We're also not the first to have issues with that change - https://github.com/symfony/symfony/issues/25341 - unfortunately it doesn't look like the fix fixed it for us.

Mile23’s picture

Should we make 3.3 work while we're waiting?

alexpott’s picture

@Mile23 3.3 has the same problem - we need to solve the recursion in OptimizedPhpArrayDumper or discover why is it happening.

alexpott’s picture

@Mile23 also for the purposes of progress here we can just change default.services.yml from

  factory.keyvalue:
    {}
    # Default key/value storage service to use.
    # @default keyvalue.database
    # default: keyvalue.database
    # Collection-specific overrides.
    # state: keyvalue.database
  factory.keyvalue.expirable:
    {}
    # Default key/value expirable storage service to use.
    # @default keyvalue.database.expirable
    # default: keyvalue.database.expirable
  # Allowed protocols for URL generation.

to

  factory.keyvalue: {}
    # Default key/value storage service to use.
    # @default keyvalue.database
    # default: keyvalue.database
    # Collection-specific overrides.
    # state: keyvalue.database
  factory.keyvalue.expirable: {}
    # Default key/value expirable storage service to use.
    # @default keyvalue.database.expirable
    # default: keyvalue.database.expirable
  # Allowed protocols for URL generation.

That's enough to make our test run run (one we've fixed the recursion) BUT the problem is that default.services.yml is used to copy to a services.yml for many sites so they're all going to have this problem and asking everyone to check and fix their custom services.yml for 8.5.0 feels really really wrong.

alexpott’s picture

Issue summary: View changes

Updated issue summary to detail the upstream issue in Symfony - see https://github.com/symfony/symfony/issues/25786

alexpott’s picture

Version: 8.5.x-dev » 8.6.x-dev
Status: Needs work » Needs review
FileSize
2.59 KB
68.12 KB

Somethings still not right with the container but posting work for others to maybe see a way forward. The change to private services seems to have affected us. We have to get this in 8.6.x first so changing version.

Status: Needs review » Needs work

The last submitted patch, 35: 2927746-35.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.17 KB
69.3 KB

Our dumper does not seem to like some of the changes to the symfony's container. We've got a problem with parameters as arguments. Patch attached seems to fix so that at least drupal installs and can install modules without crashing.

Status: Needs review » Needs work

The last submitted patch, 37: 2927746-37.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.52 KB
69.62 KB

Here's some added deprecations - we should try and resolved as many of of them as possible here.

Status: Needs review » Needs work

The last submitted patch, 39: 2927746-39.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.13 KB
73.57 KB

Here's a couple removed.

Status: Needs review » Needs work

The last submitted patch, 41: 2927746-41.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.21 KB
74.28 KB
catch’s picture

Updating the actual code for the X-Status-code stuff.

edit: crosspost issues but think the right patch made it anyway..

The last submitted patch, , failed testing. View results

The last submitted patch, 44: 2927746-43.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 44: 2927746-44.patch, failed testing. View results

alexpott’s picture

Status: Needs work » Needs review
FileSize
8.97 KB
78.35 KB

A few more fixes.

alexpott’s picture

Few more fixes.

The last submitted patch, 48: 2927746-48.patch, failed testing. View results

alexpott’s picture

catch’s picture

+++ b/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php
@@ -186,7 +194,32 @@ public function getListenerPriority($eventName, $listener) {
   public function hasListeners($event_name = NULL) {
-    return (bool) count($this->getListeners($event_name));
+    if ($event_name !== NULL) {
+      if (empty($this->listeners[$event_name])) {
+        return FALSE;
+      }
+      else {
+        foreach ($this->listeners[$event_name] as $priority => $listeners) {
+          if (!empty($listeners)) {
+            return TRUE;
+          }
+        }
+        return FALSE;
+      }
+    }
+    if (!empty($this->listeners)) {
+      foreach ($this->listeners as $event_name => $event_listeners) {
+        if (!empty($event_listeners)) {

$event_name isn't used again, but it's a bit strange overwriting the method param. Also all this logic could use an explanation why it's needed.

Everything else looks OK. I'm tempted to commit to both branches with the option to revert from 8.5.x if we later decide the YAML parsing regression is too disruptive, since for me security support outweighs people having to deal with that regression - we can fix the regression with a patch updated to Symfony in a patch release, but can't do the minor update.

alexpott’s picture

One possibility is to leave Yaml at 3.2 until it is fixed. It will be super disruptive. I'll have a look at the event code later. Not sure what's going on there yet myself.

catch’s picture

Leaving YAML on 3.2 sounds like a good compromise.

larowlan’s picture

Pinning yaml until the upstream fix works for me, assuming we can remove it before 8.5.0

jibran’s picture

$event_name isn't used again, but it's a bit strange overwriting the method param. Also all this logic could use an explanation why it's needed.

This change is needed to keep up with upstream EventDispatcher changes. In #2909185: Deprecate ContainerAwareEventDispatcher in favor of SymfonyEventDispatcher, we are looking to completely remove/deprecate it after this issue is fixed.

larowlan’s picture

+++ b/core/tests/Drupal/Tests/Component/Serialization/YamlTest.php
@@ -90,7 +92,25 @@ public function testObjectSupportDisabled() {
+    // objects so in order to encode an object we hace to use the PECL

nit: have

this looks good to me, we just need to discuss how to handle the services.yml issue

alexpott’s picture

Keeping symfony/yaml on 3.2 doesn't work :(

    "conflict": {
        "symfony/config": "<3.3.7",
        "symfony/finder": "<3.3",
        "symfony/proxy-manager-bridge": "<3.4",
        "symfony/yaml": "<3.4"
    },

from vendor/symfony/dependency-injection/composer.json

In fact because of the conflicts in Symfony component composer.json files we'd only we able to update symfony/class-loader, symfony/http-foundation and symfony/process to 3.4 because all the others have inter-dependent conflicts if you leave them on 3.4

pounard’s picture

I think that upgrading the whole to 3.4 is much better for compatibility with potential other libraries module developers would like to use, it would much less be a dependency hell.

I'd prefer to have to fix a few yaml files during upgrade rather than be blocked by a too lower dependency. That's of course just an opinion and that matter still needs to be discussed, but I'd prefer to see Drupal move toward the future than being held back :)

alexpott’s picture

Addressed #57 and #52. I've made the way \Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::removeListener() works more similar to the way Symfony's does which in turn allows \Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::hasListeners() to be an exact copy of the upstream code.

@pounard I think this just means we need to resolve the upstream fix before we go forward.

jibran’s picture

If we think https://github.com/symfony/symfony/issues/25786 is a correct fix and will be merged upstream then why not create a fork and pin the commit/branch in composer.json and we can update composer.json anytime before March 7 release date for 8.5.0. The idea here is to commit it to 8.5.x ASAP so that contrib can adapt and we can find as many bugs as possible before 8.5.0.

Removing the needs followup tag as I don't see anything which should be moved to followups also we need a change record(s) for all the API changes.

alexpott’s picture

@jibran at the very least we need follow-ups to decide what to do with each deprecation notice:

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -279,6 +279,12 @@ public static function getSkippedDeprecations() {
+      'The Symfony\Component\ClassLoader\ApcClassLoader class is deprecated since Symfony 3.3 and will be removed in 4.0. Use `composer install --apcu-autoloader` instead.',
+      'The Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher class is deprecated since Symfony 3.3 and will be removed in 4.0. Use EventDispatcher with closure factories instead.',
+      'The Symfony\Component\HttpFoundation\Session\Storage\Handler\WriteCheckSessionHandler class is deprecated since Symfony 3.4 and will be removed in 4.0. Implement `SessionUpdateTimestampHandlerInterface` or extend `AbstractSessionHandler` instead.',
+      'The "session_handler.write_check" service relies on the deprecated "Symfony\Component\HttpFoundation\Session\Storage\Handler\WriteCheckSessionHandler" class. It should either be deprecated or its implementation upgraded.',
+      'Not setting the strict option of the Choice constraint to true is deprecated since Symfony 3.4 and will throw an exception in 4.0.',
+      'Using the Yaml::PARSE_KEYS_AS_STRINGS flag is deprecated since Symfony 3.4 as it will be removed in 4.0. Quote your keys when they are evaluable instead.',

Here are some thoughts on each deprecation:

  • I think we need to copy the ApcClassLoader to core and not deprecate. Mandating composer install --apcu-autoloader is not possible whilst we support non-composer builds.
  • ContainerAwareEventDispatcher seems to have a follow-up.
  • I think we need to implement our own "session_handler.write_check" service
  • The Choice one is really painful if I remember correctly as I think it means FormAPI changes
  • Yaml::PARSE_KEYS_AS_STRINGS is also quite painful because we are trying to support both Symfony and PECL yaml.
alexpott’s picture

Let's fix PHP 5.5. This also brings up that Symfony's container has had a lot of work and new features for 3.4 - for example https://github.com/symfony/symfony/pull/24872 - that might offer performance benefits to Drupal too.

catch’s picture

Cross posting - I was also looking at the PHP 5.5 failures. Interdiff touches the same methods (against #60), but I copied the function bodies across from Symfony's container too since they're supposed to be 1-1 copies.

alexpott’s picture

I don't think #64 will work because doResolveServices is private to the parent class. Discussed with @catch and we agreed to add @todos and file a followup to look at \Drupal\Core\DependencyInjection\ContainerBuilder and the upstream changes. Filed #2937010: Bring \Drupal\Core\DependencyInjection\ContainerBuilder inline with \Symfony\Component\DependencyInjection\Container and apply 3.4 improvements.

Going back to #63 and adding @todos + fixing PHP7.2.

catch’s picture

hmm #64 actually does pass, but we might not be hitting that code path...

jibran’s picture

Thanks, for explanation @alexpott.
Restoring the tag as per #62. On less critical note we also have #2922862: Update non-Symfony dependencies in lock file.

catch’s picture

Discussed with @xjm briefly. Is it worth looking at subclassing the Yaml and Parse classes to avoid a fork? Looks like we have to override at least three methods

mpdonadio’s picture

I think long term subclassing (and marking @internal and maybe @deprecated, too) would be better so that they don't get out into the wild.

alexpott’s picture

Well I think that Symfony will go forward with the upstream PR so potentially we could just work around our immediate problem like this.

Maybe we should do this in \Drupal\Component\Serialization\Yaml::decode() but that'll have a much bigger performance impact.

Status: Needs review » Needs work

The last submitted patch, 70: 2927746-69.patch, failed testing. View results

alexpott’s picture

alexpott’s picture

pounard’s picture

Instead of hardcoding services to public, would it be worthwhile to mark them as public in the services definition files ?

Symfony 3.4 allows to set defaults parameters for services in files, such as (exemple from vanilla Symfony install):

services:
    # default configuration for services in *this* file
    _defaults:
        autowire: true      # Automatically injects dependencies in your services.
        autoconfigure: true # Automatically registers your services as commands, event subscribers, etc.
        public: false       # Allows optimizing the container by removing unused services; this also means
                            # fetching services directly from the container via $container->get() won't work.
                            # The best practice is to be explicit about your dependencies anyway.

This would be a small BC for contrib modules, but well documented it will be very easy for maintainers to fix them. And it also would be a first shy step toward Drupal using the new container API properly.

alexpott’s picture

@pounard that would break sites during update - all modules using services would require a change. Also I don't think the _defaults in not supported by Drupal's container at the moment. We'd need to make changes to our version of YamlFileLoader also every \Drupal::service() call would have to be replaced. It's just too disruptive for us. Although I totally agree that we should make these changes for Drupal 9.

alexpott’s picture

The reason for the PHP5 fails is that the PHP5 bots don't have the yaml extension installed. Here are fixes for the fails.

andypost’s picture

catch’s picture

Status: Needs review » Reviewed & tested by the community

I think we should still try to get this into the 8.5.x alpha if at all possible. We're going to be over a year out of security support for Symfony if we don't get it into 8.5.0. There are some workarounds here but the YAML one will be resolved once the upstream PR is in and we managed to avoid forking and we have a couple of follow-ups created already for YAML and the container changes.

xjm’s picture

This still needs a change record.

xjm’s picture

larowlan’s picture

Adding review credits

  • larowlan committed 47c2dda on 8.6.x
    Issue #2927746 by alexpott, catch, slasher13, mpdonadio, martin107,...

  • larowlan committed 568053d on 8.5.x
    Issue #2927746 by alexpott, catch, slasher13, mpdonadio, martin107,...
larowlan’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed as 47c2dda and pushed to 8.6.x.

Cherry-picked as 568053d and pushed to 8.5.x

Published change record, see you in the follow-ups folks.

Great work here.

webchick’s picture

This patch seems like kind of a big deal. :)

pounard’s picture

I opened a few issues under the 'be nice with symfony' tag such as #2909185: Deprecate ContainerAwareEventDispatcher in favor of SymfonyEventDispatcher and #2832025: Most code using the container builder are wrongly type-hinted - most of them are about removing Drupal specific code and use Symfony's one instead. In the long run, it will make Symfony's upgrade much much easier. And it also allows me to actually spawn the fullstack and merge it into Drupal core within some projects - I can use Symfony's forms, security component, and bundles within Drupal (believe me, that's fun and nice at the same time).

Mile23’s picture

CR refers to 8.4.x.

jibran’s picture

mondrake’s picture

After this commit an unsilenced deprecation occurs when running PHPUnit tests on Windows:

The Symfony\Component\ClassLoader\WinCacheClassLoader class is deprecated since Symfony 3.3 and will be removed in 4.0. Use `composer install --apcu-autoloader` instead.

alexpott’s picture

@mondrake can you file a follow up issue to add that to \Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecations() plus add this to the followup to #2937534: Bring the Symfony\Component\ClassLoader\ApcClassLoader into core and undeprecate so that we can duplicate that class too.

jhedstrom’s picture

I just ran into an issue where a module that was decorating another module's services started failing after this update. The failure message was not clear ("You have requested a non-existent service ..."). The fix was to manually mark those decorating services as public. This decoration takes place via a service provider class rather than via a YAML file.

It is probably worth mentioning this in the change notice?

larowlan’s picture

Yes please @jhedstrom if you could add that to the change notice that'd be great!

jhedstrom’s picture

@larowlan done!

larowlan’s picture

thanks

alexpott’s picture

@jhedstrom can you post a link to the module? Ideally there should not have to be a change. I think we might have to override \Symfony\Component\DependencyInjection\ContainerBuilder::setDefinition() to handle publicness.

jhedstrom’s picture

@alexpott it's this patch on the group module that needed adjusting: #2906085: Content moderation integration: transition permissions and latest version access

tim.plunkett’s picture

jibran’s picture

tim.plunkett’s picture

After this, I started to get the error from #2887000: composer.json does not constrain Symfony components to minor and patch versions that are compatible with Drupal again:

Error: Class 'Symfony\Component\Config\Resource\ComposerResource' not found in ~/www/d8/vendor/symfony/dependency-injection/ContainerBuilder.php

alexpott’s picture

@tim.plunkett I've tried the steps in the issue summary on #2887000: composer.json does not constrain Symfony components to minor and patch versions that are compatible with Drupal on 8.6.x / PHP 7.1 to reproduce the error you are seeing but it didn't happen for me :(

tim.plunkett’s picture

PHP 7.1.11
Composer 1.5.2

~/www/d8$ rm -rf vendor
~/www/d8$ composer install
Loading composer repositories with package information...
...
~/www/d8$ find . -name ComposerResource.php
~/www/d8$ composer require drush/drush
Using version ^9.0 for drush/drush
./composer.json has been updated
Loading composer repositories with package information...
...
~/www/d8$ find . -name ComposerResource.php
./vendor/symfony/config/Resource/ComposerResource.php

I'm guessing you had drush installed when you tested. And most of us (and even the testbot?!) will also have drush installed.
But that file, which Symfony's ContainerBuilder expects, does not exist in a stock install.

Mile23’s picture

I'm guessing you had drush installed when you tested. And most of us (and even the testbot?!) will also have drush installed.

No drush in the testbot.

alexpott’s picture

@tim.plunkett I don't have the ComposerResource locally. Looking at Symfony's ContainerBuilder it looks like this would only be used if setResourceTracking() has been called with TRUE. We set that to false in \Drupal\Core\DependencyInjection\ContainerBuilder::__construct(). But that call it pretty useless because later in the parent it does:

$this->trackResources = interface_exists('Symfony\Component\Config\Resource\ResourceInterface');

I think what is happening here is that Drush has the dependency so container built with drush get resource tracking set to TRUE but containers built by phpunit or webhead don't. Created #2940956: Properly disable resource tracking in the ContainerBuilder to address this.

Status: Fixed » Closed (fixed)

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