Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 UTC on 18 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#76 | 2927746-76.patch | 87.09 KB | alexpott |
#76 | 73-76-interdiff.txt | 3.38 KB | alexpott |
Comments
Comment #2
slasher13Copied metadata from issue #2874909: Update Symfony components to 3.3.*
Interdiff to https://www.drupal.org/project/drupal/issues/2874909#comment-12338961
https://www.drupal.org/project/drupal/issues/2874909#comment-12365917 Done
Comment #4
slasher13require symfony/debug ~3.4.0
Comment #5
dawehner@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?
Comment #7
slasher13First 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.
Comment #8
jonathanshawComment #9
slasher13Most failures because of
Turn services and aliases private by default, with BC layer and
Make as many services private as possible
More changes:
https://symfony.com/blog/symfony-3-4-curated-new-features:
Comment #10
martin107 CreditAttribution: martin107 as a volunteer commented@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
Comment #11
martin107 CreditAttribution: martin107 as a volunteer commentedNeeded reroll.. I won't retest.
Comment #12
moshe weitzman CreditAttribution: moshe weitzman commentedCan 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
Comment #13
slasher13No, changed in symfony 3.3.14 update:
see https://www.drupal.org/files/issues/interdiff_27770.txt
from #2874909: Update Symfony components to 3.3.*
Comment #14
moshe weitzman CreditAttribution: moshe weitzman commentedThanks 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.Comment #15
RoSk0Re-roll.
Comment #16
mpdonadioLooks 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.
Comment #26
cilefen CreditAttribution: cilefen commentedI 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:
Thank you everyone for the continued efforts. Getting on Symfony 3.4 components will be a good thing.
Comment #27
alexpottSo 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.Comment #28
pounardI 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.
Comment #29
alexpottThe 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.Comment #30
alexpottWe'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.
Comment #31
Mile23Should we make 3.3 work while we're waiting?
Comment #32
alexpott@Mile23 3.3 has the same problem - we need to solve the recursion in OptimizedPhpArrayDumper or discover why is it happening.
Comment #33
alexpott@Mile23 also for the purposes of progress here we can just change default.services.yml from
to
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.
Comment #34
alexpottUpdated issue summary to detail the upstream issue in Symfony - see https://github.com/symfony/symfony/issues/25786
Comment #35
alexpottSomethings 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.
Comment #37
alexpottOur 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.
Comment #39
alexpottHere's some added deprecations - we should try and resolved as many of of them as possible here.
Comment #41
alexpottHere's a couple removed.
Comment #43
alexpottComment #44
catchUpdating the actual code for the X-Status-code stuff.
edit: crosspost issues but think the right patch made it anyway..
Comment #48
alexpottA few more fixes.
Comment #49
alexpottFew more fixes.
Comment #51
alexpottComment #52
catch$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.
Comment #53
alexpottOne 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.
Comment #54
catchLeaving YAML on 3.2 sounds like a good compromise.
Comment #55
larowlanPinning yaml until the upstream fix works for me, assuming we can remove it before 8.5.0
Comment #56
jibranThis change is needed to keep up with upstream EventDispatcher changes. In #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher, we are looking to completely remove/deprecate it after this issue is fixed.
Comment #57
larowlannit: have
this looks good to me, we just need to discuss how to handle the services.yml issue
Comment #58
alexpottKeeping symfony/yaml on 3.2 doesn't work :(
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
Comment #59
pounardI 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 :)
Comment #60
alexpottAddressed #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.
Comment #61
jibranIf 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.
Comment #62
alexpott@jibran at the very least we need follow-ups to decide what to do with each deprecation notice:
Here are some thoughts on each deprecation:
composer install --apcu-autoloader
is not possible whilst we support non-composer builds.Yaml::PARSE_KEYS_AS_STRINGS
is also quite painful because we are trying to support both Symfony and PECL yaml.Comment #63
alexpottLet'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.
Comment #64
catchCross 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.
Comment #65
alexpottI don't think #64 will work because doResolveServices is private to the parent class. Discussed with @catch and we agreed to add
@todo
s and file a followup to look at \Drupal\Core\DependencyInjection\ContainerBuilder and the upstream changes. Filed #2937010: Bring ContainerBuilder inline with Symfony Container and apply upstream improvements.Going back to #63 and adding
@todo
s + fixing PHP7.2.Comment #66
catchhmm #64 actually does pass, but we might not be hitting that code path...
Comment #67
jibranThanks, for explanation @alexpott.
Restoring the tag as per #62. On less critical note we also have #2922862: Update non-Symfony dependencies in lock file.
Comment #68
catchDiscussed 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
Comment #69
mpdonadioI think long term subclassing (and marking @internal and maybe @deprecated, too) would be better so that they don't get out into the wild.
Comment #70
alexpottWell 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.
Comment #72
alexpottMissed a place.
Comment #73
alexpottInteresting that PHP5 exercises a code path not exercised by PHP7.
Comment #74
pounardInstead 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):
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.
Comment #75
alexpott@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.Comment #76
alexpottThe reason for the PHP5 fails is that the PHP5 bots don't have the yaml extension installed. Here are fixes for the fails.
Comment #77
andypostThere's existing bug with yaml decode #2792877: Symfony YAML parser fails on some strings when running PHP 7
Comment #78
catchI 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.
Comment #79
xjmThis still needs a change record.
Comment #80
xjmComment #81
larowlanFollow ups for #62
#2937534: Bring the Symfony\Component\ClassLoader\ApcClassLoader into core and undeprecate
#2937537: Remove uses of Symfony\Component\EventDispatcher\ContainerAwareEventDispatcher from core
#2937540: Remove usages of Symfony\Component\HttpFoundation\Session\Storage\Handler\WriteCheckSessionHandler
#2937541: [PP-1] The "session_handler.write_check" service relies on the deprecated WriteCheckSessionHandler class
#2937542: 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
#2937543: Using the Yaml::PARSE_KEYS_AS_STRINGS flag is deprecated since Symfony 3.4 as it will be removed in 4.0
Draft CR https://www.drupal.org/node/2937545
Comment #82
larowlanAdding review credits
Comment #85
larowlanCommitted 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.
Comment #86
webchickThis patch seems like kind of a big deal. :)
Comment #87
pounardI opened a few issues under the 'be nice with symfony' tag such as #2909185: Replace ContainerAwareEventDispatcher with Symfony EventDispatcher 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).
Comment #88
Mile23CR refers to 8.4.x.
Comment #89
jibranCreated #2937984: [META] Symfony 4.0 compatibility.
Comment #90
mondrakeAfter 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.
Comment #91
alexpott@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.Comment #92
mondrakeFiled #2938369: The Symfony\Component\ClassLoader\WinCacheClassLoader class is deprecated, and added a comment + postponed #2937534: Bring the Symfony\Component\ClassLoader\ApcClassLoader into core and undeprecate.
Comment #93
jhedstromI 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?
Comment #94
larowlanYes please @jhedstrom if you could add that to the change notice that'd be great!
Comment #95
jhedstrom@larowlan done!
Comment #96
larowlanthanks
Comment #97
alexpott@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.
Comment #98
jhedstrom@alexpott it's this patch on the group module that needed adjusting: #2906085: Content moderation integration: transition permissions and latest version access
Comment #99
tim.plunkettOpened #2939208: Services registered with ContainerBuilder::setDefinition() are removed from container causing a BC break for 93-98
Comment #100
jibranAdded #2940367: Update Symfony components to 3.4.4.
Comment #101
tim.plunkettAfter 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:
Comment #102
alexpott@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 :(
Comment #103
tim.plunkettPHP 7.1.11
Composer 1.5.2
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.
Comment #104
Mile23No drush in the testbot.
Comment #105
alexpott@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:
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.