Problem/Motivation
At some point we need to update to symfony 3.0, as this will be the only supported version.
Proposed resolution
- For now let's move to 2.8 to cover all new features / deprecations.
- Commit this patch to 8.1
- Announce the update to 2.8
- Announce the update to 3.0 in 8.2, so custom/contrib code can adapt to it
Other commends
Problems with update to 3.0 identified as part of this issue:
Let's experiment whether/how we can update.
Issues/problems identified:
- https://github.com/stackphp/builder/pull/16
- https://github.com/symfony-cmf/Routing/issues/123
- Our test clones a container, in the patch
- Our dispatcher test extends the symfony class but our implementation doesn't implement a specific interface.
- vendor/symfony/serializer/Encoder/JsonDecode.php is borked, calling to a wrong function: https://github.com/symfony/symfony/pull/16581
- We use prototype services for integration with zend feed ... maybe replace them with an immutable "implementation"
- Symfony removed the teapot!!!! https://github.com/symfony/symfony/pull/16585
Remaining tasks
User interface changes
API changes
Data model changes
| Comment | File | Size | Author |
|---|---|---|---|
| #88 | update_to_symfony_2_8-2611816-88-quickfix.patch | 482 bytes | neclimdul |
| #80 | interdiff.txt | 1.72 KB | dawehner |
| #80 | 2611816-80.patch | 777.04 KB | dawehner |
| #78 | interdiff-2611816-67-78.txt | 936 bytes | ankit agrawal |
| #78 | 2611816-78.patch | 784.8 KB | ankit agrawal |
Comments
Comment #2
dawehnerFirst problem:
Comment #3
dawehnerNext problem:
Comment #4
dawehnerComment #5
dawehnerLet's ask the testbot.
Comment #7
cosmicdreams commentedHey @dawehner I'm hearing form PHP folks that https://github.com/symfony/symfony/issues/9406 might be a significant issue as it's the "biggest backwards compatibility break" from their opinion.
Comment #8
dawehnerIt wouldn't matter much because we just use the http kernel side of http kernel, none of the other stuff.
Comment #9
dawehnerUploading an update to symfony 2.8
Comment #11
dawehnerListing a couple of problems in the issue summary.
Comment #12
clifford8 commentedComment #13
dawehnerLet's update with some fixes.
Comment #15
catchIt's out: http://symfony.com/blog/symfony-3-0-0-released
Would be great to get this into 8.1.x sooner than later.
Also would be completely fine with an incremental update to 2.8.0, then a further issue to get from there to 3.0.0
Comment #16
dawehner2.8 to 3.0 should be trivial, because 2.8 throws all the deprecation notices, but yeah I don't care which one to target.
Comment #17
dawehnerUpdated to the 2.8 release, this should fix most of the issues, but still not all of them.
Comment #20
dawehnerThis is actually really crazy. We kinda have a problem I guess?
Comment #21
catchSo this is http://symfony.com/blog/new-in-symfony-2-8-deprecated-service-definitions
But then we'll get a different deprecation notice when that service is instantiated.
In Drupal we've got a distinction between marking something @deprecated, and using trigger_error() per #2575081: [policy, no patch] Use E_USER_DEPRECATED in Drupal 8 minor releases - so that we can give people the earliest possible warning of a deprecation, then start showing errors in a future minor release.
So we're going to need to disable Symfony's deprecation framework as far as I can see - at least for now.
Then if we want to use it later, we'd need to be able to add an opt-out to the trigger_error() in service definitions.
Something like
Comment #22
pounardIf the goal to pursue is to upgrade to SF3, all deprecation warnings must be fixed, not silenced, since the 2.8 deprecation warnings are explicitly telling you what have been removed in 3.0. I myself did that migration from 2.7 to 3.0 in personal SF projects, 2.8 and 3.0 are ISO in everything except that 3.0 removes all deprecated features, and 2.8 keep them only in the goal of making migration from 2.7 easier.
Drupal should not upgrade to 2.8 IMHO, I think this issue is doing it right by first upgrading to 2.8 in a temporary fashion, just the time to fix all the deprecation warnings, once done, Drupal will be able to seamlessly go to 3.0 in this very same issue without ever 2.8 being commited in any of the 8.x branch(es).
Comment #23
catch@pounard if you read the error, it's not a Symfony 2.8 deprecation but a Drupal deprecation we added just before release. We explicitly do not want to trigger errors for these yet to give contrib and custom code time to update. Similarly Symfony did not start triggering errors for deprecations until 2.8.
The problem is that deprecations we've added are being picked up by Symfony's deprecation system, not that we're using deprecated Symfony classes.
Comment #24
dawehnerFirst I think we should actually support deprecated services, which we didn't see the interdiff. Once we are there, we can explicitly opt out something.
Comment #25
pounard@catch Oh I didn't catch that, sorry for the noise.
Comment #26
jibranComment #28
dawehnerFixed the remaining failure.
Comment #30
dawehnerExpanded the test coverage of
\Drupal\Tests\Component\DependencyInjection\Dumper\OptimizedPhpArrayDumperTestComment #31
dawehnerFirst a patch for the core only changes
Note: We cannot yet update to symfony 3.0, due to
a)
vendor/twig/twig/composer.json:34: "symfony/debug": "~2.7"b)
vendor/symfony-cmf/routing/composer.json:17: "symfony/routing": "~2.2",c)
vendor/stack/builder/composer.json:15: "symfony/http-kernel": "~2.1"Comment #33
dawehnerYeah that patch was not meant to be tested
Comment #34
catch@dawehner pointed out in irc that doing 2.8 in 8.1.x then 3.x in 8.2.x gives contrib modules a full release to clean up any deprecation notices, which makes sense to me.
Comment #35
dawehnerRerolled. Thank you @berdir for mentioning that!
Comment #36
Crell commentedBy 8.2 we'd probably be looking at Symfony 3.1 or 3.2. Other than that note I think #34 makes sense and is consistent with what we've discussed for our own deprecated code elsewhere.
Comment #37
catch#34 say 3.x, not 3.0, so not sure what you mean?
Comment #38
dawehnerYeah we would go with
~3.0, whatever that is when8.1is outComment #39
Crell commentedI think we're all saying the same thing.
Comment #40
catchWhile checking when the first 3.x LTS would be, I noticed #2598662-28: Decide when Drupal 7 (and Drupal 8 and later) should enter the LTS and security-support-only phase.
Comment #41
dawehnerAre we waiting for something on this issue actually? Just being curious, we could be in something like "code freeze" for 8.1.x still?
Comment #42
hussainwebFixing a really small typo. I think 'provider' was a typo and I was going to change it to 'provide', but I think 'define' is a better word here. Thoughts?
Comment #43
catch@dawehner for me this is an issue which could go into 8.1.x whenever, no need to wait.
Comment #44
dawehnerWell, this is a copy from symfony itself.
Comment #45
hussainwebI knew other parts were a copy. I didn't know the test too was a copy. I am not sure what to do. For sake of consistency, we could just let it be the same as typo...
Comment #46
dawehnerI just could not care less about that bit :)
Comment #47
dawehnerRerolled against no vendor dir. Its so much better
Comment #49
hussainwebThere is probably something wrong with how composer is run.
Comment #50
dawehnerOh I somehow assumed that we got rid of the vendor dir already. Here is a patch with it.
Comment #52
dawehnerAnother try.
Comment #54
dawehnerMhh, maybe --binary helps
Comment #57
mile23Just a heads-up that some issues with YAML are fixed in 2.8: #2591827: Add YAML linting to core coding standards checks
Comment #59
jibranReroll after #1475510: Remove external dependencies from the core repo and let Composer manage the dependencies instead.
Comment #60
jibranStupid typo.
Comment #63
dawehnerRerolled
Comment #64
jibranHere is core only patch for review.
Comment #65
jibranIt just has cloned vendor code updates so seems ready to me.
Comment #66
catchThis could do with some more explanation.
Something like:
Also needs a change notice.
Tagging needs re-roll/novice for those
Otherwise looks great to me and would be good to get in asap.
Comment #67
dawehnerAdded a change record... and fixed that one comment.
Comment #68
jibran/me RTBC immediately.
Comment #69
catch@alexpott pointed out this looks bit odd, and I agree.
Comment #70
dawehnerCan't agree more.
Comment #71
catchJust trying without that hunk. #69 reversed is the interdiff...
Comment #73
dawehnerWell, reverting is wrong, but maybe
$service['shared'] = $definition->isShared()is enough. I mean to be clear, what actually matters is the non shared case, as this is the non default behaviour.Comment #74
catchYes that makes sense, just wanted to be sure it was absolutely necessary at all and not cruft.
Just with that change.
Comment #76
dawehnerOH yeah, now I even remember why I made exactly that change. Setting the shared flag always a) increases the stored container and b) requires a lot of changes in the test coverage of the optimized container. I could just not be bothered to change them.
Comment #77
ankit agrawal commentedComment #78
ankit agrawal commentedRe-rolled the patch with above changes. Please review. Thanks
Comment #80
dawehnerIMHO exposing shared all the time is still wrong. This is optimized, so we try to minimize the size of each entry
I'll revert that for now and just expand the test coverage
Comment #81
dawehnerComment #82
catchThe comment in #81 is probably all we needed there, thanks.
Comment #83
catchCommitted/pushed to 8.1.x, thanks! Great that this got in with a bit of time to spare before the 8.1.0 beta.
Comment #85
dawehnerNice!
Comment #86
penyaskitoCreated #2674780: Unexpected API change in UrlGenerator::generate()
Comment #87
neclimdulInvalid Yaml
Comment #88
neclimdulThis fixes it. The only way I have to suggest confirming it is pasting the yaml here: http://www.yamllint.com/ (sorry its slow) or grabbing the patch in #1920902-268: Add a Drupal Yaml wrapper so we can default to PECL Yaml component if it is available and running the phpunit unit tests with pecl yaml installed.
Comment #89
tstoecklerLooks good to me.
Comment #90
dawehner+1
Comment #92
catchCommitted/pushed to 8.1.x, thanks!
Comment #94
xjmComment #95
jibranCreated #2703319: Update to symfony/css-selector 2.8 as follow up and there is a new one #2702677: Update to Symfony 2.8.4.