Recommended commit message
git commit -m 'Issue #2470693 by Bedir, dawehner, jibran, znerol, hussainweb, pwolanin: Upgrade to Symfony 2.7.0'
Symfony 2.7 beta is out. I think we should upgrade since we are tracking stables.
This issue basically checks if all the tests pass with Symfony 2.7. This is the changelog.
Beta phase evaluation
Issue category | Task because it is an external library upgrade. |
---|---|
Issue priority | Critical because Symfony 2.7.x is required for PHP7 support and because 2.7.x is an LTS that be used throughout D8's lifecycle. |
Disruption | Not disruptive. |
Comment | File | Size | Author |
---|---|---|---|
#77 | 2470693-77.patch | 3.09 MB | dawehner |
#76 | interdiff.txt | 2.22 KB | znerol |
#70 | interdiff.txt | 5.67 KB | dawehner |
#70 | 2470693-70.patch | 3.09 MB | dawehner |
#65 | 2470693-64.patch | 3.04 MB | Berdir |
Comments
Comment #1
hussainwebI should have pulled before creating. Anyway, I think we can always reuse for Symfony 2.7.
Comment #2
hussainwebComment #3
hussainwebAdding a composer only and a full patch.
Comment #4
hussainwebComment #5
TJacksonVA CreditAttribution: TJacksonVA commentedWe should also note that the Symfony folks have changed their strategy slightly. Rather than going from 2.7 to 3.0, they will also have an additional 2.8 release which will come out at the same time as Symfony 3.0 in November, Transition from Symfony 2.7 to 3.0... Symfony 2.8 on its way. The Symfony 2.8 release will also be a LTS release like Symfony 2.7, so we may want to consider as we approach November whether we want to upgrade to Symfony 2.8. However, we do not need to make that decision at this time.
Comment #6
hussainwebIs there a problem with the test? I see that it has been reset and retested by administrator several times.
Comment #7
pfrenssenYes the test bot doesn't seem to like it. It has been queued half a dozen times now.
Comment #9
BerdirNot sure what the error before was, but the patch currently doesn't apply anymore.
Comment #10
hussainwebI will try to get to it by today evening.
Comment #11
TJacksonVA CreditAttribution: TJacksonVA commentedThey Symfony folks have just released BETA2 of Symfony 2.7. Symfony 2.7.0-BETA2 released. The stable production release of Symfony 2.7 is still currently scheduled for end of May/beginning of June.
I think this issue needs to be upgraded to Critical. Drupal 8 is currently on Symfony 2.6.x, which will end-of-life for bug fixes in July 2015, and end-of-life for security fixes in January 2016. It is essential (critical?) that we move on to Symfony 2.7 as soon as possible. In [policy, no patch] Follow symfony 2.7 or 3.0., this is the question, we agreed that when Symfony 2.7 beta came out we would upgrade to it, and then track additional Symfony 2.7 releases as we do additional releases of Drupal 8 betas (and then stables).
Given that Symfony 2.6 is very close to its end-of-life, I do not believe that we can do a Drupal 8 RC with Symfony 2.6, so from my perspective it is critical that we update to Symfony 2.7.
Comment #12
jibranI tried to update to Symfony 2.7.0-BETA2 here https://qa.drupal.org/pifr/test/1045813
Comment #13
dawehner@TJacksonVA
Yeah no question at all, we should follow the symfony releases, even to 2.8 but its not a reason to put it to critical.
Its just yet antother release.
Interesting result! Opened up a follow up in #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation
Comment #14
dawehner#2489122: Get rid of calls to deprecated API calls.
Comment #15
dawehnerLet's merge stuff together and see whether it works
Comment #17
pwolanin CreditAttribution: pwolanin at Acquia commentedposting big patch for dawehner
Comment #19
dawehnerComment #21
jibranToo much exceptions 8-)
Comment #22
dawehnerComment #25
jibranGreat work much less exceptions.
Comment #26
BerdirFixed the fatal error in the unit tests (yay, symfony is using PSR-4 now, 3 directory levels less in vendor/symfony), this should hopefully allow us to see the test fails.
Still some unit test fails that I wasn't able to figure out quickly. Mostly deprecation messages, with _method and some validator stuff.
Hint: use
git config --global diff.renameLimit 1500
so that rename detection works with that many files, cuts the patch size in half.Comment #28
BerdirGoing through some of those exceptions...
WTF @getMessagePluralization() being deprecated in favor of getPlural() but that isn't in the interface. real funny ;) Same for getMessageParameters(). I think the deprecation messages might actually be on the wrong method?
Comment #30
dawehnerSome more fixes ...
Comment #32
dawehnerQuick reroll.
Comment #34
dawehnergit diff 8.0.x --binary
Comment #36
dawehnerFixed the remaining once.
Comment #38
jibranThis might apply correctly.
Comment #39
jibranHere is reviewable patch.
Comment #40
jibranComment #41
dawehnerHere is a change record: https://www.drupal.org/node/2489948
Comment #42
tim.plunkettSo we just don't need this anymore? Fine by me
This seems scary, and needs a comment. (and maybe tests?!)
?!
Comment #43
dawehnerWell, we the method and schema are used later, so its really about the _format being accidentally unset.
As explained, DefaultTranslator got marked as deprecated. Note: The .gitignore file which existed before is now tracked as part
symfony/validator
itself.Comment #44
dawehnerForgot the comment.
Comment #45
BerdirDo you have more information what this is about?
As I said above, the new methods aren't defined on the interface and are documented as alias of the old ones.
Either the documentation needs to be updated to reflect the deprecation or it needs to be changed (e.g., the other methods should have the trigger_error()). So I'm pretty sure we need a PR and should create that before they release the stable release, but I'm not sure what the PR should actually do...
Comment #46
dawehnerOH right I totally forgot to tell you about that. When I talked with fago about the weirdness of the validator he told me that webmozart plans to get rid of the interface and rely entirely on a value object here in 3.0 so we are kinda prepared here and its fine what we are doing. The fact that its not part of the interface is caused by having a BC promise on the interface.
Comment #47
tim.plunkettI went through this today with @dawehner, and I think he adequately addressed @Berdir's question in #45. I consider it RTBC.
Comment #48
jibranWe can mark
as won't fix now.
Comment #49
Berdir#46: Ok, fine, but the documentation there is really bad :(
This is still 2.7 BETA, I don't think we want to commit this already?
The PHP7 fix is also AFAIK not yet part of this.
It's great that it's green now, so I'd suggest to postpone/wait for the final release and then do the update?
We might also need a change record for some of the changes caused by this and the beta evaluation doesn't seem correct, there is definitely distruption?
Comment #50
znerol CreditAttribution: znerol commentedAs this patch updates our forked
YamlFileLoader
, it would be great if it also would include the changes missed during the 2.5 and 2.6 updates #2375339: Update DependencyInjection YamlFileLoader for Symfony 2.6.Comment #51
catchYes we should only update to the stable, great that this is green though.
Also CNW for #50.
Comment #52
catchSince this required updates to get working, bumping to critical - core shouldn't be relying on deprecated Symfony code for 8.0.0.
Comment #53
dawehnerAccording to the symfony release schedule we will have an update to the stable version in may, so let's just wait a bit more and get it done.
Comment #54
pfrenssenPostponed awaiting the Symfony 2.7 LTS release. See http://symfony.com/roadmap.
Comment #55
jibranIf this is postponed till the stable release then can we re-open #2486883: Upgrade to Symfony 2.6.7?
Comment #56
dawehnerWhat would be the gain? The LTS release will be there in a short time.
Comment #57
neclimdulSince 2.6.8 contain security fixes.
Comment #58
neclimdulComment #59
jibranReroll after #2493807: Add symfony/console component to core
Comment #60
dawehner2.7.0 got tagged, see https://github.com/symfony/symfony/commit/7493c2bef54fb818c5304bdd9d2194...
Comment #61
TJacksonVA CreditAttribution: TJacksonVA commentedand officially released: Symfony 2.7.0 released
Comment #63
BerdirFixed the test fail.
This also means that TypedDataTest is now passing on PHP7, this should be the last real test fail.
Comment #65
BerdirRenamed our class and generated patch with --binary.
Comment #66
BerdirComment #68
jibranAs per #47.
Comment #69
znerol CreditAttribution: znerol commentedWhat about #50/#51?
Comment #70
dawehnerSure, let's do that. Patch btw. nearly applied.
Comment #71
dawehnerUpdated the recommended commit message ...
Comment #72
xjm(Saving issue credit to correspond to the proposed commit message.)
Comment #73
xjm@webchick, @alexpott, @catch, @effulgentsia and I all agreed this was critical for its impacts on both #2400407: [meta] Ensure vendor (PHP) libraries are on latest stable release (part of #2485119: [meta] The Drupal 8.0.0-rc1 Release Checklist) and #2454439: [META] Support PHP 7, and D8 core needs to use a version of Symfony that will also be supported throughout the D8 cycle.
Leaving this one for someone else to commit though. :) And yay!
Comment #74
xjmComment #75
znerol CreditAttribution: znerol commenteddiff -u core/vendor/symfony/dependency-injection/Loader/YamlFileLoader.php core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
Still is shows hunks which look like upstream changes which need to be replicated into our
YamlFileLoader
. Will examine them and post another patch soon.Comment #76
znerol CreditAttribution: znerol commentedOnly posting interdiff here. I have no idea on how to properly produce the whole diff (mine results in 13M but #70 is only 3M).
Comment #77
dawehnerGood catch, I guess those changes happened in the meantime since december?
@znerol
The trick is to use
git diff --binary
Comment #78
BerdirThe trick is to use git diff --binary *and* have git renames configured *and* git config --global diff.renameLimit 1500, see #26 :)
Comment #79
BerdirInterdiff seems fine to me there were no other changes, lets get this in.
Comment #80
alexpottI think we should completely rip out support for synchronised services in our container. This patch removes some of the test coverage since the methods throw PHP warnings (because deprecated).
Getting this is quickly because if it causes pain in any other criticals it is good to know early - same goes for contrib. Committed 73b5652 and pushed to 8.0.x. Thanks!
Comment #83
jibran#2504967: Upgrade to Symfony 2.7.1
Comment #84
Fabianx CreditAttribution: Fabianx as a volunteer and at Tag1 Consulting commentedPublished the change record (yes, a bit late, but still important ...)
Comment #85
willzyx CreditAttribution: willzyx commentedThe upgrade has caused #2695109: Cache bins are not deleted when the module that declares them is uninstalled