Problem/Motivation

Symfony 3 is here, we decided to adopt it for Drupal 8 in #2395443: [policy, no patch] Follow symfony 2.7 or 3.0., this is the question.

Proposed resolution

While the current patch passes tests, Drush must be updated to use Symfony 3.2 as well - see this PR: https://github.com/drush-ops/drush/pull/2477

Updating to Symfony 3.2 means that Drush will need to update its PHP requirement from PHP 5.4 to PHP 5.5.9 to match Symfony's. This likely requires a new major release of drush, since people on older PHP installs should not update to that new version. There may also be further contributed module updates for Symfony 3 compatibility.

Due to this, we're planning to commit this patch to 8.4.x on May 2nd 2017 and monitor any additional breakage over time. The hope is that Drupal 8.4.0 will ship with an up-to-date release of Symfony 3, but Drupal 8.3.x will stay on Symfony 2.8.* to avoid disruption to contrib and sites.

Remaining tasks

Fix #2712633: Update symfony-cmf/routing to 1.4.0.
Fix #2712637: Update stack/builder for Symfony 3 compatibility.
Fix #2712643: Update behat/* to 1.7.1 and fabpot/goutte to 3.1.2.
Fix #2720869: Remove the use of deprecated CssSelector and use CssSelectorConverter instead.
Fix #2720891: Replace ContainerAware with ContainerAwareTrait
Fix #2721139: Replace deprecated files ParameterBag usage.
Fix #2721179: Replace deprecated Symfony ExecutionContextInterface.
Fix #2726645: Do not @deprecate EntityManager until all usages are removed (helps the Symfony 3 update).
Fix #2728815: Batch API uses request attributes instead of query in Symfony 3.
Fix #2730129: DrupalKernel must never persist service_container for Symfony 3 update.
Fix #2682373: Implement ContainerAwareEventDispatcher::getListenerPriority().
Fix #2776105: Update asm89/stack-cors for Symfony 3 compatibility and revert changes introduced by #1869548.
Fix #2823687: Use isMethodCacheable() instead of isMethodSafe() when checking for GET/HEAD.
Fix fails.
Review.
Commit.

User interface changes

None

API changes

Hopefully none

Data model changes

Not yet

Files: 
CommentFileSizeAuthor
#355 2712647-355.patch91.15 KBmpdonadio
#347 interdiff-343-347.txt14.57 KBmpdonadio
#347 2712647-347.patch91.14 KBmpdonadio
#343 2712647-343.patch91.33 KBslasher13
#341 2712647-341.patch46.31 KBwebflo
#337 2712647-337.patch91.33 KBtimmillwood
#335 update_symfony-2712647-335.patch91.39 KBslasher13
#333 update_symfony-2712647-333.patch91.48 KBslasher13
#333 interdiff.txt404 bytesslasher13
#331 update_symfony-2712647-331.patch91.52 KBslasher13
#330 update_symfony-2712647-330.patch94.64 KBslasher13
#329 update_symfony-2712647-329.patch94.63 KBslasher13
#327 interdiff.txt1.68 KBslasher13
#327 update_symfony-2712647-327.patch92.95 KBslasher13
#323 interdiff.txt7.27 KBslasher13
#323 update_symfony-2712647-323.patch94.63 KBslasher13
#322 update_symfony-2712647-322.patch94.53 KBslasher13
#322 interdiff.txt3.23 KBslasher13
#320 interdiff.txt18.26 KBslasher13
#320 update_symfony-2712647-320.patch91.41 KBslasher13
#319 update_symfony-2712647-319.patch91.41 KBslasher13
#318 interdiff.txt13.68 KBslasher13
#318 update_symfony-2712647-318.patch91.43 KBslasher13
#312 interdiff.txt17.44 KBslasher13
#312 update_symfony-2712647-312.patch91.43 KBslasher13
#311 interdiff.txt29.22 KBslasher13
#311 update_symfony-2712647-311.patch91.38 KBslasher13
#310 update_symfony-2712647-310.patch92.51 KBslasher13
#301 update_symfony-2712647-301.patch94.38 KBslasher13
#301 interdiff.txt16.64 KBslasher13
#297 update_symfony-2712647-297.patch94.38 KBjibran

Comments

jibran created an issue. See original summary.

jibran’s picture

jibran’s picture

Issue summary: View changes
Status: Needs work » Postponed

Postponing it now on child issues.

dawehner’s picture

Thank you for creating this issue.

In theory we could directly update because we fixed all BC throws in 2.8, but I just doubt that :)

jibran’s picture

Status: Postponed » Needs review
FileSize
50.18 KB

Maybe we should jump ahead and start fixing the errors.

jibran’s picture

Status: Needs work » Needs review
FileSize
60.99 KB
jibran’s picture

Status: Needs work » Needs review
FileSize
2.33 KB
63.31 KB
dawehner’s picture

The question is basically: how long do we want people to adapt to it. To be clear, the usages of old API (<2.8) got removed from core, but contrib / custom code might still be out there.
When we add it to 8.2, we would choose 6 months ... maybe a year might be better, maybe not.

Eric_A’s picture

the usages of old API (<2.8) got removed from core, but contrib / custom code might still be out there.

Is there a reason the change record was left in draft or did we just forget to publish it?

Something else: if we update drupal/core composer.json we should also update drupal/core-* composer.json files whenever components depend on symfony. I've opened #2720739: [meta] drupal/core-* components and drupal/core requires/replace declarations are broken, which is about the missed "2.7.*" to "~2.8" updates.

dawehner’s picture

r did we just forget to publish it?

That sounds much more likely.

klausi’s picture

Issue summary: View changes

I think we should close this issue as "won't fix". Symfony 3 has many API breaks, see https://github.com/symfony/symfony/blob/master/UPGRADE-3.0.md

API break examples: the ContainerAware class was removed, which is used in core and might potentially be used by contrib modules. The _method setting on routes has been renamed to _methods, so this could potentially break many of our routing definitions (if we use the Symfony routing code part for this, I have not checked).

Drupal 8 will stay on Symfony 2.8 LTS forever, because that is the non API breaking promise of Drupal 8. Symfony 3+ is Drupal 9 material.

dawehner’s picture

We decided before that given the LTS of Drupal is supported for much longer than the LTS of symfony, so there is no way around that: https://www.drupal.org/node/2403135

dawehner’s picture

The _method setting on routes has been renamed to _methods, so this could potentially break many of our routing definitions (if we use the Symfony routing code part for this, I have not checked).

Well, this throws deprecation notes, which is a fatal in any test we have. Its weird that ContainerAware doesn't throw anything.

klausi’s picture

Issue summary: View changes

Apologies, I missed #2395443: [policy, no patch] Follow symfony 2.7 or 3.0., this is the question where it was already decided that Drupal 8 will at some point adopt Symfony 3.

The looser is obviously Drupal 8 contrib and custom site modules because they will experience API breaks.

In Drupal 7 we tried to avoid API breaks at all costs, so this is a somewhat cultural change that we need to communicate more.

dawehner’s picture

The looser is obviously Drupal 8 contrib and custom site modules because they will experience API breaks.

Well yeah ideally symfony 2.8 would have thrown deprecation notices for everything, and then, as said before, we should wait at least for a year to update, if not more.

jibran’s picture

Issue summary: View changes

We need another follow up for

Fatal error: Class 'Symfony\Component\CssSelector\CssSelector' not found in /var/www/html/core/modules/simpletest/src/AssertContentTrait.php on line 250
09:55:42 Recording test results

.
Created #2720869: Remove the use of deprecated CssSelector and use CssSelectorConverter instead.

catch’s picture

We should either put this into 8.2.x very soon so there's plenty of time to find and fix bc breaks in contrib/custom. Or wait until 8.3.x is open and do it then to give them even longer.

We essentially said that stuff deprecated in 2.8 was not part of the public Drupal API so people should not be using that or can fix their usages if they are.

jibran’s picture

The looser is obviously Drupal 8 contrib and custom site modules because they will experience API breaks.

Perhaps create some BC library or module.

Xano’s picture

Would code be able to be fully compatible with both versions of Symfony, even though that is likely a bad idea? If not, that means sites will be forced to go through the trouble of upgrading to 8.2 if they want to receive security updates for core or contrib.

I am concerned about what this does to Drupal's image and its adoption. This is another break of our Semantic Versioning, and even though this change was announced long ago, people still won't care that we did, as it breaks their products as soon as they run composer update, even though their constraints were all correct.

If we do go ahead with this, I suggest we do it right now, or right after 8.2 is released, so contrib has close to six months to test and upgrade.

jibran’s picture

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

Would code be able to be fully compatible with both versions of Symfony, even though that is likely a bad idea?

2.8/3.0 theoretically have 100% compatible APIs, 3.0 just removed things that were already deprecated in 2.8. We'll find out how true that is in practice.

We should probably commit a patch here or in a side issue that is just fixing core to work with 3.0 (i.e. without the update itself). If that works then contrib/custom ought to be able to do the same.

Xano’s picture

2.8 was also released late last year, so long after we were in code freeze.

dawehner’s picture

dawehner’s picture

Issue summary: View changes
jibran’s picture

I think we need another child issue for container scope clean up.

Xano’s picture

We should probably commit a patch here or in a side issue that is just fixing core to work with 3.0 (i.e. without the update itself). If that works then contrib/custom ought to be able to do the same.

Can we test this without running tests multiple times using different composer.lock files? If not, do we need to contact the DA to discuss support for this?

klausi’s picture

@Xano I think jibran's strategy is fine. We should have all the new things in 2.8 already, so we can update our usage before even updating to 3.0. Ideally this issue then only has a patch with the composer updates and nothing breaks. No additional DA support necessary.

jibran’s picture

We can't move Note: The $scope parameter is deprecated since version 2.8 and will be removed in 3.0. to child because in 2.8 \Symfony\Component\DependencyInjection\ContainerInterface contains scope methods:

    public function enterScope($name);
    public function leaveScope($name);
    public function addScope(ScopeInterface $scope);
    public function hasScope($name);
    public function isScopeActive($name);

All these methods are removed in Symfony 3 so to remove these from core we have to bump Symfony.

jibran’s picture

Status: Needs work » Needs review
FileSize
6.92 KB
75.51 KB

Some more work.

klausi’s picture

Issue summary: View changes
dawehner’s picture

All these methods are removed in Symfony 3 so to remove these from core we have to bump Symfony.

I'm happy that we got rid of any of our usages of scope though.

klausi’s picture

klausi’s picture

Status: Needs work » Needs review
FileSize
90.22 KB
14.71 KB

Rolling in those 2 issues to see how far we get now on the testbot.

klausi’s picture

Status: Needs work » Needs review
FileSize
91.79 KB
1.57 KB

Removed one more scope test in ContainerTest.

klausi’s picture

Status: Needs work » Needs review
FileSize
91.77 KB

Auto git merge with 8.2.x, no other changes.

klausi’s picture

Assigned: Unassigned » klausi

I should really execute the phpunit tests locally instead of waiting 110 minutes until the testbot aborts. Wow, our phpunit testbot runner is quite broken.

klausi’s picture

while grepping for IntrospectableContainerInterface I found #2721315: Theme registry FastTest is using a non-initialized container.

klausi’s picture

Status: Needs work » Needs review
FileSize
96.83 KB
5.07 KB

More fixes for the moved ValidatorInterface and the removed IntrospectableContainerInterface.

Something is broken for me with the phpunit execution when running

../vendor/bin/phpunit --stop-on-failure
PHP Fatal error:  Call to undefined function Drupal\Core\EventSubscriber\error_displayable() in core/lib/Drupal/Core/EventSubscriber/DefaultExceptionSubscriber.php on line 75

Looks like includes/errors.inc is not included when it should have been, let's see what the testbot thinks.

dawehner’s picture

Looks like includes/errors.inc is not included when it should have been, let's see what the testbot thinks.

How is that related with symfony 3.0, mhhhh

jibran’s picture

Well I tried to run phpunit on trvias every since we started working ktbng. Now after JTB I ran all the phpunit tests on travis again here. @klausi this might help you.

klausi’s picture

Status: Needs work » Needs review
FileSize
83.46 KB
1.84 KB

Some more progress.

* HTTP methods are now not part of the requirements of a route anymore in Symfony 3.
* Request::create() ingores _format query parameter.

klausi’s picture

Assigned: klausi » Unassigned

And I merged in 8.2.x. Feel free to take this over from here.

jibran’s picture

Do we have to fix these as well?

grep -inr '?_format=' core/
core/tests/Drupal/Tests/Core/Routing/RequestFormatRouteFilterTest.php:74:    $request = Request::create('test?_format=xml', 'GET');
core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php:56:    $path_alias_storage->save('/conneg/html?_format=json', '/alias.json');
core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php:72:      ['conneg/html?_format=json', '', 'application/json'],
core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php:73:      ['conneg/html?_format=json', '*/*', 'application/json'],
core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php:74:      ['conneg/html?_format=json', 'application/xml', 'application/json'],
core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php:75:      ['conneg/html?_format=json', 'application/json', 'application/json'],
core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php:76:      ['conneg/html?_format=xml', '', 'application/xml'],
core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php:77:      ['conneg/html?_format=xml', '*/*', 'application/xml'],
core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php:78:      ['conneg/html?_format=xml', 'application/json', 'application/xml'],
core/modules/system/src/Tests/Routing/ContentNegotiationRoutingTest.php:79:      ['conneg/html?_format=xml', 'application/xml', 'application/xml'],
klausi’s picture

klausi’s picture

Current status: update.php batching is broken. Somehow Drupal returns HTML when the batch JS expects JSON. This seems to be another instance of the problem that Request in Symfony 3 does not understand _format=json in URL query parameters. I have not figured out yet how and where this is different.

You can test this by adding an empty system_update_8015() function to system.install. Then go to /update.php and Try to run updates => kaboom.

klausi’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
85.58 KB
574 bytes
klausi’s picture

Status: Needs work » Needs review
FileSize
86.17 KB
1.15 KB
Berdir’s picture

+++ b/core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php
@@ -159,6 +156,19 @@ public function getListeners($event_name = NULL) {
    */
+  public function getListenerPriority($eventName, $listener) {
+    if (isset($this->listeners[$eventName])) {
+      foreach ($this->listeners[$eventName] as $priority => $listeners) {
+        if (($key = array_search($listener, $listeners, TRUE)) !== FALSE) {
+          return $priority;
+        }

See also #2682373: Implement ContainerAwareEventDispatcher::getListenerPriority()

klausi’s picture

klausi’s picture

Issue summary: View changes

Thanks Berdir, another depending issue we need to solve.

jibran’s picture

Issue summary: View changes

Nice work. Some minor points.

+++ b/core/modules/link/tests/src/Unit/Plugin/Validation/Constraint/LinkAccessConstraintValidatorTest.php
@@ -29,7 +29,7 @@ class LinkAccessConstraintValidatorTest extends UnitTestCase {
+    $context = $this->getMock('Symfony\Component\Validator\Context\ExecutionContextInterface');

+++ b/core/modules/link/tests/src/Unit/Plugin/Validation/Constraint/LinkExternalProtocolsConstraintValidatorTest.php
@@ -19,7 +19,7 @@ class LinkExternalProtocolsConstraintValidatorTest extends UnitTestCase {
+    $context = $this->getMock('Symfony\Component\Validator\Context\ExecutionContextInterface');

@@ -77,7 +77,7 @@ public function testValidateWithMalformedUri() {
+    $context = $this->getMock('Symfony\Component\Validator\Context\ExecutionContextInterface');

@@ -97,7 +97,7 @@ public function testValidateIgnoresInternalUrls() {
+    $context = $this->getMock('Symfony\Component\Validator\Context\ExecutionContextInterface');

+++ b/core/modules/link/tests/src/Unit/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidatorTest.php
@@ -19,7 +19,7 @@ class LinkNotExistingInternalConstraintValidatorTest extends UnitTestCase {
+    $context = $this->getMock('Symfony\Component\Validator\Context\ExecutionContextInterface');

@@ -94,7 +94,7 @@ public function testValidateWithMalformedUri() {
+    $context = $this->getMock('Symfony\Component\Validator\Context\ExecutionContextInterface');

+++ b/core/modules/user/tests/src/Unit/Plugin/Validation/Constraint/ProtectedUserFieldConstraintValidatorTest.php
@@ -47,7 +47,7 @@ public function testValidate($items, $expected_violation, $name = FALSE) {
+    $context = $this->getMock('Symfony\Component\Validator\Context\ExecutionContextInterface');

We can use class constant here.

klausi’s picture

Status: Needs work » Needs review
FileSize
92.46 KB
13.24 KB

Yes, switched to ::class syntax and merged in changes from child issues.

klausi’s picture

Title: Update Symfony components to 3.0 » [PP-5] Update Symfony components to 3.0
Issue summary: View changes
Status: Needs review » Postponed

Perfect, now this is postponed on all the remaining child issues.

Removed #2729247: Replace remaining $request->get() usage to prepare core for Symfony 3 because it is not a hard requirement for the Symfony 3 update.

klausi’s picture

Status: Postponed » Needs review
FileSize
91.17 KB
8.36 KB
klausi’s picture

Title: [PP-1] Update Symfony components to 3.0 » Update Symfony components to 3.0
Issue tags: +Needs reroll

catch just committed the last one, so this is ready to be rerolled again.

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
80.33 KB

Fixed conflicts on:

  • composer.lock - stack/builder was bumped to v1.0.4, however this patch sets it to dev-master. I've kept our changes here, although I'm not entirely sure about this decision.
  • core/lib/Drupal/Component/EventDispatcher/ContainerAwareEventDispatcher.php
  • core/modules/system/src/Tests/DrupalKernel/DrupalKernelTest.php
klausi’s picture

Status: Needs review » Needs work
  1. +++ b/composer.lock
    @@ -1091,16 +1049,16 @@
                 "name": "stack/builder",
    -            "version": "v1.0.4",
    +            "version": "dev-master",
    

    nope, that should not happen. stack/builder should stay on 1.0.4 which is Symfony 3 compatible.

  2. +++ b/core/composer.json
    @@ -25,7 +25,7 @@
    -        "stack/builder": "1.0.*",
    +        "stack/builder": "dev-master",
    

    same here, that should not be changed.

Manuel Garcia’s picture

Assigned: Unassigned » Manuel Garcia

Right, let me try that again...

dawehner’s picture

  1. +++ b/core/composer.json
    @@ -25,7 +25,7 @@
    -        "stack/builder": "1.0.*",
    +        "stack/builder": "dev-master",
    

    that is no longer needed

  2. +++ b/core/modules/rest/tests/src/Unit/CollectRoutesTest.php
    @@ -130,8 +130,8 @@ public function testRoutesRequirements() {
    -    $this->assertEquals(count($requirements_1), 0, 'First route has no requirement.');
    -    $this->assertEquals(count($requirements_2), 2, 'Views route with rest export had the format and method requirements added.');
    +    $this->assertEquals(0, count($requirements_1), 'First route has no requirement.');
    +    $this->assertEquals(1, count($requirements_2), 'Views route with rest export had the format requirement added.');
    

    If we are switching things anyway here we could use assertCount as well

Manuel Garcia’s picture

Assigned: Manuel Garcia » Unassigned
Status: Needs work » Needs review
FileSize
78.82 KB
1.69 KB

OK here's a proper reroll + that change to core/composer.json

Manuel Garcia’s picture

Status: Needs review » Needs work

Settting it to needs work based on #79.2

klausi’s picture

  1. +++ b/composer.lock
    @@ -4155,12 +3995,14 @@
    -    "stability-flags": [],
    +    "stability-flags": {
    +        "stack/builder": 20
    +    },
    

    hm, why is this still here? Can you run composer update on the symfony packages only again?

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -46,20 +46,16 @@ private function getProxyInstantiator()
       /**
    -   * Direct copy of the parent function.
    +   * Shares a given service in the container.
    +   *
    +   * @param Definition $definition
    +   * @param mixed      $service
    +   * @param string     $id
        */
    

    why are we changing that comment? I mean, it is useful to have the @param comments here, but we should also still mention that the method is a direct copy. Or is it not a direct copy anymore? Then we should also mention that and what we modified.

  3. +++ b/core/modules/rest/tests/src/Unit/CollectRoutesTest.php
    @@ -130,8 +130,8 @@ public function testRoutesRequirements() {
    -    $this->assertEquals(count($requirements_1), 0, 'First route has no requirement.');
    -    $this->assertEquals(count($requirements_2), 2, 'Views route with rest export had the format and method requirements added.');
    +    $this->assertEquals(0, count($requirements_1), 'First route has no requirement.');
    +    $this->assertEquals(1, count($requirements_2), 'Views route with rest export had the format requirement added.');
    

    Agreed with dawehner, let's use assertCount() here if we change the lines anyway.

dawehner’s picture

Agreed with dawehner, let's use assertCount() here if we change the lines anyway.

On the other hand there is not a real reason to change these as part of this issue if you are honest.

Manuel Garcia’s picture

Status: Needs work » Needs review
FileSize
82.47 KB
28.71 KB

OK my last attempt at a proper reroll here... I'll get out of your hair now :S

jibran’s picture

Title: Update Symfony components to 3.0 » Update Symfony components to 3.1.0
Status: Needs work » Needs review
FileSize
1.45 KB
83.07 KB

symfony 3.1.0 was released last week.

jibran’s picture

Issue tags: +Needs change record

Let's add a change record for this as well.

klausi’s picture

Status: Needs review » Needs work
+++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
@@ -46,20 +46,16 @@ private function getProxyInstantiator()
   /**
-   * Direct copy of the parent function.
+   * Shares a given service in the container.
+   *
+   * @param Definition $definition
+   * @param mixed      $service
+   * @param string     $id
    */
   protected function shareService(Definition $definition, $service, $id)

why are we changing that comment? I mean, it is useful to have the @param comments here, but we should also still mention that the method is a direct copy. "Direct copy of the private parent function." as additional comment I would say.

Otherwise looks good!

I checked http://symfony.com/doc/current/contributing/community/releases.html and it seems no Symfony 3.x version until 3.4 will be an LTS, so I think it is fine to upgrade until Symfony 3 gets into LTS mode. At least we are not experiencing additional breaking changes comparing 3.0 and 3.1, which is a good sign.

dawehner’s picture

At least we are not experiencing additional breaking changes comparing 3.0 and 3.1, which is a good sign.

Sadly this is not strictly right. They throw E_USER_DEPRECATED, which we convert to test failures.

naveenvalecha’s picture

Issue tags: -Needs change record

here we go with change record https://www.drupal.org/node/2743809 but it needs polishing before publishing what exact changes has been done.

naveenvalecha’s picture

Status: Needs work » Needs review
FileSize
83.07 KB
514 bytes

Addressed #88.
Added another paragraph for the same.

dawehner’s picture

  1. +++ b/core/composer.json
    @@ -5,19 +5,19 @@
    +        "symfony/class-loader": "~3.0",
    +        "symfony/console": "~3.0",
    +        "symfony/dependency-injection": "~3.0",
    +        "symfony/event-dispatcher": "~3.0",
    +        "symfony/http-foundation": "~3.0",
    +        "symfony/http-kernel": "~3.0",
    +        "symfony/routing": "~3.0",
    +        "symfony/serializer": "~3.0",
    +        "symfony/translation": "~3.0",
    +        "symfony/validator": "~3.0",
    +        "symfony/process": "~3.0",
    

    should we still explicit use "~3.1" instead? Feels a bit safer to be honest.

  2. +++ b/core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
    @@ -47,19 +47,17 @@ private function getProxyInstantiator()
       /**
        * Direct copy of the parent function.
    +   *
    +   * Shares a given service in the container.
    +   *
    +   * @param Definition $definition
    +   * @param mixed      $service
    +   * @param string     $id
        */
       protected function shareService(Definition $definition, $service, $id)
       {
    

    We could just use {@inheritdoc} and call it a day, can't we?

jibran’s picture

+1 to #92

Manuel Garcia’s picture

Addressing #92
Besides what @dawehner mentioned, also bumped the versions for symfony/yaml and symfony/css-selector

klausi’s picture

Status: Needs review » Needs work

Patch looks good.

Now we need to list all the breaking changes from child issues that we had to fix in the change record.

bojanz’s picture

https://twitter.com/dunglas/status/740123859324653568 makes it sound like we should be updating prophecy as well?

naveenvalecha’s picture

#96 +1
sounds like a followup would be good for it.

dawehner’s picture

@bojanz
If we haven't run into it yet, we won't do now. Its a pure composer.json thing, see https://github.com/phpspec/prophecy/pull/273

cosmicdreams’s picture

I don't see how #94 address the concern of being more explicit with Symfony 3.1 support with the related libraries. Many libraries specifically reference Symfony "~3.0"

dawehner’s picture

Many libraries specifically reference Symfony "~3.0"

Well that is not a problem in that sense. Its SEMV, so it will use 3.1.

Eric_A’s picture

klausi’s picture

Status: Needs work » Needs review

Completed the list of API breaks we experienced at https://www.drupal.org/node/2743809 , please review!

I think we can ignore the drupal component composer.json files in this issue and leave that to the related issue. Same for the prophecy update, should also be a separate issue.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I think its fine as it is. Note: The amount of changes is so small, because we updated to 2.8 before, which throws those exception warnings before, especially see https://www.drupal.org/node/2611816#comment-10578742 so the problems would have caught people before.

jibran’s picture

I linked the new change notice with old one which is unfortunately still in a draft state.

alexpott’s picture

Status: Reviewed & tested by the community » Needs review

After discussing with @catch we where wondering if there is a chance that the patch can be done without the library updates. This way we can prove that contrib can be compatible with 2.8 and 3.1 at the same time. This is important because otherwise modules are going to have to start adding requirements checks on core and also DrupalCI tests modules against 8.2.x so they'll need to fix that sooner.

catch’s picture

I'm particularly interested in the ExecutionContextInterface changes, but also YAML seems not impossible to run into. If we can do those without the update, then it verifies those are 2.8 deprecations and not fresh breaks.

klausi’s picture

Nope, not possible for ExecutionContext, see my comment in #2721179-2: Replace deprecated Symfony ExecutionContextInterface.

catch’s picture

So a way they could have done that, would be to leave the old interface in the old place @deprecated, and add the new one - then code could be written for both. However it feels relatively unlikely that 8.x contrib/custom code is using this, or at least the best way to find out is probably to commit this and see what breaks. If we end up with contrib modules that can't have working 8.1.x and 8.2.x versions, we could potentially fork 2.8 in 8.1.x to add back the old interface or something.

dawehner’s picture

So a way they could have done that, would be to leave the old interface in the old place @deprecated, and add the new one - then code could be written for both.

They have done that for quite a while since symfony 2.5 or so. The step onto symfony 3 was simply to remove the old deprecated class/interfaces.

catch’s picture

2.8 doesn't have the new interface though?

alexpott’s picture

@catch it does have the new interfaces but it's not so useful (unfortunately)... you get errors like...

Declaration of Drupal\link\Plugin\Validation\Constraint\LinkAccessConstraintValidator::initialize() must be compatible with Symfony\Component\Validator\ConstraintValidatorInterface::initialize(Symfony\Component\Validator\ExecutionContextInterface $context) in /Volumes/devdisk/dev/sites/drupal8alt.dev/core/modules/link/src/Plugin/Validation/Constraint/LinkAccessConstraintValidator.php on line 15

And this is because the new interface does not extend the old one and so ConstraintValidatorInterface::initialize() can't be satisfied :(

So yeah this patch problem does contain the minimum amount of change. The container changes are tricky too because they remove a param from ContainerInterface::set() so it's not possible just make the changes to our container.

So given this the question no becomes how do we manage this change for the affect contrib modules?

klausi’s picture

Contrib modules that are affected by one of this breaking changes have 2 choices:
1) Make their code compatible with 8.2.x and tell users they must update to 8.2.x (otherwise their sites break on a stable 8.1.x)
2) Start a new branch with a new major version, for example Rules 8.x-3.x compatible with core 8.1.x and Rules 8.x-4.x compatible with core 8.2.x.

We should probably put that in the change notice?

jibran’s picture

Start a new branch with a new major version, for example Rules 8.x-3.x
compatible with core 8.1.x and Rules 8.x-4.x compatible with core 8.2.x.

We should really start semantic versioning for contrib as well. As a module developer, I'm not convinced of releasing a new major version of module due to new Drupal core minor release. Unfortunately, this leave us with Make their code compatible with 8.2.x and tell users they must update to 8.2.x (otherwise their sites break on a stable 8.1.x) and I'd rather do latter then former as module developer.

klausi’s picture

Looked into the YAML parse() change, but we cannot do that beforehand since there is no replacement in Symfony 2.8. It got simply replaced in 3.0, so it has to be done with this issue.

Managing contrib modules: I think we cannot do anything except documenting the API breaks - the good thing is that probably only a small portion of contrib modules will be affected. I would say we should rather commit this sooner than later so that contrib has time to catch up with the API breaks until 8.2.0 is released and make the decision if they raise their minimum core requirement or start a new major branch.

Let's give this a couple of days for any input we might be missing and then put it back to RTBC.

catch’s picture

Status: Needs review » Reviewed & tested by the community

No I agree with #114. If there had been a change which would affect a lot of contrib modules, then I'd want to strongly consider hacking forward-compatibility into 8.1.x somehow (like subclassing something in both branches and encouraging people to use that instead, or forking 2.8). However there's only theoretical breakage of a small number of modules here, and we won't find out without committing it. I think we'll get more feedback if this is RTBC so moving it there, but I won't commit for a couple of days (if someone else wants to before I get to it that's up to them).

catch’s picture

Status: Reviewed & tested by the community » Postponed

Spoke to @alexpott about this and he was concerned about the validator change.

I think I have a workaround which would allow for 8.1.x and 8.2.x compatible modules.

+++ b/core/modules/system/tests/modules/theme_test/src/EventSubscriber/ThemeTestSubscriber.php
@@ -17,7 +17,8 @@ class ThemeTestSubscriber implements EventSubscriberInterface {
diff --git a/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php b/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php

diff --git a/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php b/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php
index 5905e12..246628a 100644

index 5905e12..246628a 100644
--- a/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php

--- a/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php
+++ b/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php

+++ b/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php
+++ b/core/modules/user/src/Plugin/Validation/Constraint/UserMailRequired.php
@@ -4,7 +4,7 @@

@@ -4,7 +4,7 @@
 
 use Symfony\Component\Validator\Constraint;
 use Symfony\Component\Validator\ConstraintValidatorInterface;
-use Symfony\Component\Validator\ExecutionContextInterface;
+use Symfony\Component\Validator\Context\ExecutionContextInterface;
 
 /**

This interface change means if you're using it, you definitely can't have a module that works in both 8.1.x and 8.2.x. I doubt we can do anything upstream, but we can provide a bridge in core.

Would suggest a new issue to do the following:

- Add a new, empty, ExecutionContextnterface to core.

- In 8.1.x it extends the old Symfony interface

- In 8.2.x it extends the new Symfony interface.

- Update all these use statements to use the new interface in core.

Then the actual 3.1.0 patch shouldn't need to change anything in any of these modules, and we've proved it's viable for contrib.

If you're calling methods that have changed then you're out of luck, but core isn't and we can only do so much.

Not pretty but meh. Requires modules to update, but not to branch.

Here's the issue, postponing this one again #2746337: Provide bridge between 2.8 and 3.0 ExecutionContextInterface.

jibran’s picture

Title: Update Symfony components to 3.1.0 » [PP-1] Update Symfony components to 3.1.1

3.1.1 is out now.

klausi’s picture

Title: [PP-1] Update Symfony components to 3.1.1 » [PP-1] Update Symfony components to ~3.1
Status: Postponed » Needs review
FileSize
83.02 KB
19.2 KB

Reroll to check if this still passes. Updated to Symfony components 3.1.2.

In #2746337: Provide bridge between 2.8 and 3.0 ExecutionContextInterface we had the idea of class aliases, but they cannot be developed in a separate issue and have to be done in this issue as part of the update.

klausi’s picture

I just tested the class_alias() patch from #2746337: Provide bridge between 2.8 and 3.0 ExecutionContextInterface on top of this Symfony 3 patch by executing the validation unit tests with ../vendor/bin/phpunit tests/Drupal/Tests/Core/Validation/.

PHP Fatal error:  Cannot use Symfony\Component\Validator\Context\ExecutionContextInterface as ExecutionContextInterface because the name is already in use in vendor/symfony/validator/ConstraintValidator.php on line 14

So it seems to me that the class_alias() call registers the name ExecutionContextInterface globally and then PHP fatals as soon as it encounters a use statement using ExecutionContextInterface. Maybe I'm doing something wrong?

jibran’s picture

t - 19 days till 8.2.0-beta1 will be released.

klausi’s picture

Title: [PP-1] Update Symfony components to ~3.1 » Update Symfony components to ~3.1

Since we have not found any way around the minor interface API breaks I think we have to move on with the current patch.

@jibran: can you review the patch and set this back RTBC if you approve?

jibran’s picture

Status: Needs review » Reviewed & tested by the community

After #103 the only change in #118 is version bump so it is still RTBC.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

Ah, the good old migrate module random test fails. Do we have an issue for those, since they appear on every 20th testrun or so?

Resetting to RTBC, assuming testbot will pass now.

klausi’s picture

Hm, maybe the migrate test fails are not so random after all?

Let's fix all the bugs in migrate, starting with #2767503: MigrateAggregatorStubTest typo in test namespace.

klausi’s picture

MigrateUpgrade7Test was always failing for me locally (with Symfony 2 and 3), I finally figured out why: #2767595: "-" should be treated as valid character in MySQL database names.. *facepalm*

klausi’s picture

Status: Needs work » Needs review
FileSize
84.43 KB
1.41 KB

I can't reproduce the MigrateUpgrade7Test fail locally and can't see on the testbot what assertions went wrong because of the fatal error.

Let's try again with more robust migrate tests that should at least not fatal.

jibran’s picture

Less then a week to go till we hit the beta.

slasher13’s picture

Title: Update Symfony components to ~3.1 » [PP-1] Update Symfony components to ~3.1
Issue summary: View changes
Status: Needs work » Needs review
FileSize
91.78 KB
9.24 KB

New conflicts from https://www.drupal.org/node/1869548.
Apply patch from https://www.drupal.org/node/2776105

interdiff to #118

Check if this still passes. Postpone on https://www.drupal.org/node/2776105

slasher13’s picture

slasher13’s picture

Status: Needs work » Needs review
FileSize
92.54 KB
763 bytes

Add OPTIONS and TRACE to the list of safe methods. Added to v2.8.9, too.
https://github.com/symfony/symfony/pull/19321

Down to 3 failures.

slasher13’s picture

Title: [PP-1] Update Symfony components to ~3.1 » Update Symfony components to ~3.1
Status: Needs work » Needs review
FileSize
82.81 KB
18.23 KB
  • interdiff to #118
  • update symfony/* to 3.1.3
  • add OPTIONS and TRACE to the list of safe methods

Need help with the last failures!

neclimdul’s picture

+++ b/core/tests/Drupal/Tests/Core/EventSubscriber/DefaultExceptionSubscriberTest.php
@@ -25,7 +25,8 @@ public function testOnExceptionWithUnknownFormat() {
-    $request = Request::create('/test?_format=bananas');
+    $request = Request::create('/test');
+    $request->setRequestFormat('bananas');

You shouldn't do this. Format handling is broken as is evident by the other failing tests and this is part of that.

neclimdul’s picture

https://github.com/symfony/symfony/issues/8966
https://github.com/symfony/symfony/commit/7115c1e1d913dfccbe14d5eb8f515d... is where this is broke. Symfony explicitly removed the functionality we use for format handling :(

neclimdul’s picture

NM, I forgot we didn't trust that code and wrote NegotiationMiddleware to handle setting the format. Someone might have even known this was coming I don't remember. The test was just lazy so here's a fix and documented why both cases set the format directly. This should fix the tests.

As a review, there are quite a few unrelated code reorganizations in tests that make the patch look a bit bigger that it is but things seem in order.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Great! I ran composer update symfony/* locally and get the same composer.lock outcome.

The rest of the updates look good as well, thanks for the comments about the request format middleware!

I did minor updates to the change record at https://www.drupal.org/node/2743809 and I think we are ready to get this into 8.3.x. Not sure about 8.2.x, but since we are in beta already we probably do not want this disruptive change there?

dawehner’s picture

Just went through the patch and the changes looked sane. The changes in core/lib/Drupal/Component/DependencyInjection also look complete. The file did not had unused methods or so.

Eric_A’s picture

I'd love to see this get in now, but let's also make sure we have a plan to fix the version conflicts with our sub-packages fast enough: we should fix our drupal/core-* embedded-but-destined-to-be-split-off components composer.json files here if workable, or create a quickly to be resolved follow-up issue. See #2744357-17: drupal/core-* components Symfony requirements conflict with drupal/core for reasoning why we didn't just change everything to ~2.8. With the same reasoning applied here we should not just change everything to ~3.1. Most of these symfony requirements must change to something like >= 2.7. (Now at ~2.7 or ~2.8.) Changed components that now actually require ~3.1 must of course declare ~3.1.

And perhaps we need an issue to have a test for our sub-packages that detects symfony version conflicts with drupal/core.

TJacksonVA’s picture

Any chance of getting a waiver and getting this into the next beta of 8.2.x? It would be a shame to wait until 8.3.x in order to get this kind of significant change in. For developers working on modules and new websites, they would be better served to have Symfony 3.x as soon as possible, rather than wait until 8.3.x when will be upgrading to Symfony 3.2 or 3.3 (which will be an even bigger jump at that time, as opposed to going from Symfony 3.1.x to 3.2.x or 3.3.x).

dawehner’s picture

I believe people overestimate the amount of changes from symfony 2.8 to symfony 3, especially given that symfony 2.8 has most of of the APIs already.
Given that I'm not convinced that rushing it into 8.2.x would be necessarily a good idea. Feel free to disagree with me :)

klausi’s picture

Status: Needs work » Reviewed & tested by the community

I think that was a random testbot fail, queued another test run.

alexpott’s picture

I've not seen the random fail before.

catch’s picture

Re #148 I mostly agree with this, except the core patch shows that there are certain changes where it's not possible (or at least not easy) to have code that's compatible with both Symfony 2.x and 3.x. Really comes down to ExecutionContextInterface which we've already discussed.

Question for me is whether this is going to apply to contrib modules - since DrupalCi tests contrib against 8.3.x, committing only to 8.3.x now could mean test failures on DrupalCI which when the module is updated, will break against 8.2.x. Given that, applying to both branches now, or applying to 8.3.x and 8.4.x when 8.3.x goes to beta would mean a shorter window of incompatibility.

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkNotExistingInternalConstraintValidator.php
@@ -17,7 +17,7 @@ class LinkNotExistingInternalConstraintValidator implements ConstraintValidatorI
 
diff --git a/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php

diff --git a/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
index 4f706f6..cd8a263 100644

index 4f706f6..cd8a263 100644
--- a/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php

--- a/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php

+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
+++ b/core/modules/link/src/Plugin/Validation/Constraint/LinkTypeConstraint.php
@@ -5,7 +5,7 @@

@@ -5,7 +5,7 @@
 use Drupal\link\LinkItemInterface;
 use Symfony\Component\Validator\Constraint;
 use Symfony\Component\Validator\ConstraintValidatorInterface;
-use Symfony\Component\Validator\ExecutionContextInterface;
+use Symfony\Component\Validator\Context\ExecutionContextInterface;
 

This is the change that concerns me.

Then we already discussed whether we could/should add back Symfony\Component\Validator\ExecutionContextInterface extends Symfony\Component\Validator\Context\ExecutionContextInterface to allow code to run on both versions (since we don't care about the method changes).

slasher13’s picture

Status: Needs work » Needs review
FileSize
84.16 KB
16.77 KB

update to symfony 3.1.4

slasher13’s picture

catch’s picture

Status: Needs review » Postponed

I'm going to re-open #2721179: Replace deprecated Symfony ExecutionContextInterface and postpone this issue on that - at least we should open an upstream Symfony issue to discuss the problem with them.

slasher13’s picture

catch’s picture

Status: Needs work » Postponed

Thanks for updating the patch, back to postponed.

catch’s picture

Status: Postponed » Needs review

And back to review.

klausi’s picture

Simple reroll just ignoring all hunks that don't apply anymore after #2721179: Replace deprecated Symfony ExecutionContextInterface.

klausi’s picture

Hm, I don't understand the test fails in ConfigImportAllTest, but at least I have the same test fails locally, so not a testbot issue.

klausi’s picture

The backtrace leading to the error looks very weird, AccountProxy should not load user entities when we just want the ID. See #2753733: AccountProxy can do unnecessary user loads to get an ID.

catch’s picture

catch’s picture

Status: Needs work » Needs review
FileSize
131.14 KB

Possibly a bit crufty, but here's a re-roll.

klausi’s picture

Looks like the composer.lock is messed up on PHP 5.5:

Your requirements could not be resolved to an installable set of packages.
14:38:36 
14:38:36   Problem 1
14:38:36     - Installation request for zendframework/zend-stdlib 3.1.0 -> satisfiable by zendframework/zend-stdlib[3.1.0].
14:38:36     - zendframework/zend-stdlib 3.1.0 requires php ^5.6 || ^7.0 -> your PHP version (5.5.9) does not satisfy that requirement.
14:38:36   Problem 2
14:38:36     - zendframework/zend-stdlib 3.1.0 requires php ^5.6 || ^7.0 -> your PHP version (5.5.9) does not satisfy that requirement.
14:38:36     - zendframework/zend-feed 2.7.0 requires zendframework/zend-stdlib ^2.7 || ^3.0 -> satisfiable by zendframework/zend-stdlib[3.1.0].
14:38:36     - Installation request for zendframework/zend-feed 2.7.0 -> satisfiable by zendframework/zend-feed[2.7.0].
catch’s picture

Interesting. I did this with PHP7, just running a PHP7 test to see how that goes...

klausi’s picture

Looks like your composer update was a bit too aggressive?
New patch with composer update symfony/*

klausi’s picture

Argl, why is password-compat still in there? composer--

alexpott’s picture

I guess the fail is due to \Drupal\content_translation\ContentTranslationUpdatesManager::getSubscribedEvents() - this was supposed to be fixed by #2776235: Cached autoloader misses cause failures when missed class becomes available - I wonder how and why this change is breaking that...

klausi’s picture

@alexpott: ContentTranslationUpdatesManager has a class_exists() check and the fatal error is triggered from PluginEventSubscriber in migrate module itself. So not sure how that should be related?

Did a bit of debugging in ConfigImportAllTest. It installs all modules, then exports the configuration, then uninstalls as many modules as possible, then imports the configuration again which should re-enable the modules.

When modifying the test I could trace it down to 2 modules being uninstalled before the configuration sync:

    $modules_to_uninstall = [
      'language' => TRUE,
      'migrate' => TRUE,
    ];

Some combination of language module and migrate module is at play here. language module has ConfigSubscriber which does a container rebuild onConfigSave, could that be related? Uninstalling language module also means that content translation, config translation and interface translation is uninstalled.

Whatever I tried manually in the admin backend, I could not reproduce the fatal error. It only happens in this test case. Any hints what we could try next would be appreciated.

klausi’s picture

Forgot to attach my debugging hacks.

catch’s picture

#176 PHP7 test has same number of fails, but the fail is in YAML parsing not the autoloader. I also can't reproduce the autoloader fail with PHP7 locally.

So looks like a PHP 5.5/5.6 issues with the autoloader, and then we have a separate YAML parsing PHP 7 failure.

catch’s picture

See how this does on PHP7 - fixing the YAML deprecation.

catch’s picture

FileSize
1.01 KB
alexpott’s picture

Damn negative caches especially in persistent caches like APCu. I'm not sure how we've not hit this before but anyhow. APCu will cache the class_exists miss across requests. Which means we need to add the list of modules to the prefix. This isn't happening on PHP7 because I think we've not got APCu on in that environment.

alexpott’s picture

+++ b/core/lib/Drupal/Core/DrupalKernel.php
@@ -1029,31 +1064,6 @@ protected function initializeSettings(Request $request) {
-        $loader = new ApcClassLoader($prefix, $this->classLoader);
...
-        $loader = new WinCacheClassLoader($prefix, $this->classLoader);
...
-        $loader = new XcacheClassLoader($prefix, $this->classLoader);

The alternative is to implement our our versions of these that don't negatively cache but I think the solution of using the module list is probably best.

alexpott’s picture

The biggest question about #187 is if moving the classloader swapping out to later is acceptable - I think it is because I'm not sure that anything can come in-between \Drupal\Core\DrupalKernel::initializeSettings() and \Drupal\Core\DrupalKernel::initializeContainer() so whilst there might be a few classes that are no longer loaded using the APCu (or equivalent) autoloader this number can not grow.

dawehner’s picture

+++ b/composer.lock
@@ -891,48 +891,6 @@
         {
-            "name": "ircmaxell/password-compat",
-            "version": "v1.0.4",

@@ -1720,73 +1681,20 @@
-        },
-        {
-            "name": "symfony/polyfill-apcu",

@@ -1891,143 +1799,29 @@
-        },
-        {
-            "name": "symfony/polyfill-php54",
...
-        {
-            "name": "symfony/polyfill-php55",

it is a bit confusing that those entries from the composer.lock file just disappear

klausi’s picture

@dawehner: those entries disappear because the libraries disappear because Symfony 3 does not need them anymore.

Great that alexpott found a classloader solution! We have a test fail in Drupal\datetime\Tests\Views\FilterDateTimeTest, which I could not reproduce locally. Queuing all the environments for testing again.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

@dawehner: those entries disappear because the libraries disappear because Symfony 3 does not need them anymore.

Ah gotcha, I guess Drupal doesn't use them?

tstoeckler’s picture

Interesting stuff with the classloader. One question (might be a dumb question, I don't understand all this 100%): Could this also be a problem if modules are updated and provide new classes or remove old ones?

klausi’s picture

Status: Reviewed & tested by the community » Needs work

Still test fails in ConfigImportAllTest, we need to figure those out next.

alexpott’s picture

@klausi I've run it 20 times locally - no fails... weird.

catch’s picture

I've opened #2825358: Stop using event class constants to discuss whether we really need those event classes at all.

password-compat, polyfill-php54 and polyfill-php55 are only for PHP < 5.5 support so it's correct that we don't need any of those.

Sent DrupalCI for a retest to see if those are random.

https://www.drupal.org/pift-ci-job/524064 is the fail.

catch’s picture

Status: Needs work » Needs review
FileSize
78.13 KB

Needed a re-roll, haven't managed to reproduce the random fail yet.

slasher13’s picture

Status: Needs work » Needs review
FileSize
77.03 KB

re-roll

slasher13’s picture

FileSize
4.71 KB
klausi’s picture

Status: Needs review » Needs work

Thanks a lot! The next step is to figure out the random test fails in ConfigImportAllTest.

klausi’s picture

The diff says "format_plural_string: !!binary MSBwbGFjZQNAY291bnQgcGxhY2Vz". Looks like the YAML parser/serializer changed in some way and is now spitting out "!!binary" for whatever reason?

catch’s picture

Status: Needs work » Needs review
FileSize
640 bytes
83.16 KB

See what this does.

klausi’s picture

Status: Needs review » Needs work

Ah, right, our $yaml->dump() call is wrong. We should get the exception as before so this should be

$yaml->dump($data, PHP_INT_MAX, 0, SymfonyYaml::DUMP_EXCEPTION_ON_INVALID_TYPE);
catch’s picture

alexpott’s picture

I've no idea why this is being treated as binary - it might be because it has a back-slash escaped control character. What also might be making this difficult to reproduce is that testbots are using PECL yaml for reading... which complicates things.

slasher13’s picture

It's the new binary data detection. First implemented https://github.com/symfony/symfony/pull/17863 changed in https://github.com/symfony/symfony/pull/18294. Some background informations: https://github.com/symfony/symfony/issues/18241

catch’s picture

Thanks for the links to the PRs - I found the initial one adding binary support but not the automatic binary detection yet.

So this is the relevant diff:

    '#markup' => '          format_plural_string: MSBwbGFjZQNAY291bnQgcGxhY2Vz',
      ),
      'class' => 'diff-context diff-deletedline',
    ),
    2 => 
    array (
      'data' => '+',
      'class' => 'diff-marker',
    ),
    3 => 
    array (
      'data' => 
      array (
        '#markup' => '          format_plural_string: !!binary MSBwbGFjZQNAY291bnQgcGxhY2Vz',
      ),

Don't have good ideas yet why Symfony would detect that string as binary, nor why it only happens when the data is loaded from postgres. The auto-detection for binary removed the manual flag, so we can't switch it off - either need to fix the upstream bug if there is one or somehow force this to string.

alexpott’s picture

Well the automatic binary thing must be coming from the use of the \x03 escape sequence as a plural separator... the value is "1 place\x03@count places".

hampercm’s picture

Issue summary: View changes
catch’s picture

alexpott’s picture

I think maybe the problem is that the pecl yaml reader is not able to deal with !!binary - since that'd explain the PHP7 passes above - no PECL yaml afaik.

catch’s picture

Status: Needs work » Needs review
FileSize
84.73 KB
683 bytes

This ought to do it, YAML PECL can decode binary, but doesn't by default.

catch’s picture

Assuming that passes, I think this is the right fix - Symfony's changed how their encoding works, but it's still compatible with PECL YAML with that change, plus we need to re-export the one file in core that's encoded differently.

The test failures in #220 are explicitly testing whether Symfony YAML and PECL YAML are compatible, which complements ConfigImportAll test encoding and unencoding.

Only question then for me is if we want to add some default test configuration with that format plural label rather than relying on views.view.file.yaml although that could potentially be done in a spin-off issue too.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/lib/Drupal/Core/DrupalKernel.php
    @@ -890,6 +897,34 @@ protected function initializeContainer() {
    +    // If the class loader is still the as before including settings.php use
    +    // an optimised classloader if possible.
    

    should be "... is still the same as before ..."

  2. +++ b/core/modules/config/src/Tests/ConfigImportAllTest.php
    @@ -144,6 +144,12 @@ public function testInstallUninstall() {
    +    $changelist = $storage_comparer->createChangelist()->getChangelist();
    +    foreach ($changelist['update'] as $config_name) {
    +      $diff = \Drupal::service('config.manager')->diff($this->container->get('config.storage'), $this->container->get('config.storage.sync'), $config_name);
    +      // This will fail... but then we get to see the differences.
    +      $this->assertEqual('', $this->container->get('diff.formatter')->format($diff));
    +    }
    

    this can be removed now.

  3. +++ b/core/modules/config_translation/src/Form/ConfigTranslationFormBase.php
    @@ -198,6 +198,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    +
    +      // Skip the config if it does not have translatable data.
    +      if (!isset($form_values[$name])) {
    +        continue;
    +      }
    +
    

    what about this hunk, is this still needed now?

  4. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationUiTest.php
    @@ -764,6 +765,40 @@ public function testFieldConfigTranslation() {
       /**
    +   * Tests field translation for node fields.
    +   */
    +  public function testNodeFieldTranslation() {
    

    oh, looks like you posted a patch mix up. Can you reroll the patch with just the changes for this issue?

catch’s picture

Status: Needs work » Needs review
FileSize
79.94 KB
4.28 KB

Not sure where the cruft crept in (edit, looks like #204 which explains why I didn't recognise it...). This ought to remove it though. Also fixes other points.

klausi’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -91,7 +86,10 @@ public function testResponseFormat($methods, array $supported_formats, $request_
    +      // \Drupal\Core\StackMiddleware\NegotiationMiddleware normally takes care
    +      // of this so we'll hard code it here.
    +      $request->setRequestFormat($request_format);
    

    $request_format can be FALSE, so we need to check that here before setting it. Same for the other similar changes in this file.

  2. +++ b/core/tests/Drupal/Tests/Core/PageCache/CommandLineOrUnsafeMethodTest.php
    @@ -54,8 +54,8 @@ public function providerTestHttpMethod() {
    -      [RequestPolicyInterface::DENY, 'OPTIONS'],
    -      [RequestPolicyInterface::DENY, 'TRACE'],
    +      [NULL, 'OPTIONS'],
    +      [NULL, 'TRACE'],
    

    this change conflicts with #2823687: Use isMethodCacheable() instead of isMethodSafe() when checking for GET/HEAD. Should we postpone on that issue? We could also try to use Request::isMethodCacheable() instead in CommandLineOrUnsafeMethod.php.

klausi’s picture

Looks like you lost the hunk in Drupal\Tests\rest\Unit\CollectRoutesTest, can you add that back?

catch’s picture

Status: Needs work » Needs review
FileSize
80.95 KB

Just restoring the lost hunk for now, haven't dealt with the other point yet.

catch’s picture

this change conflicts with #2823687: OPTIONS request for CORS incorrectly returned from cache with latest Symfony releases. Should we postpone on that issue? We could also try to use Request::isMethodCacheable() instead in CommandLineOrUnsafeMethod.php.

I think we could just let whichever patch land first and update the other? Symfony 3.2 should land in the next couple of weeks, bug fix support for 3.1 ends Jan 2017, postponing this issue seems unfortunate. I'm starting to think we should've just updated to Symfony 3.0 despite Symfony 3.1 coming out and opened a new issue for 3.1.

Addressed first part of #229.

klausi’s picture

Status: Needs review » Needs work

Thanks!

As next step I propose that we use Request::isMethodCacheable() in CommandLineOrUnsafeMethod.php.

Github tells me that Request::isMethodCacheable() exists since 3.1.6, so we need to set the constraint for Symfony HttpFoundation to ">=3.1.6 <4" in composer.json. https://github.com/symfony/symfony/commit/c43de7f21a587649bb42ca08bce9ad8d0c0e731d

Ideally we would also rename CommandLineOrUnsafeMethod to CommandLineOrUncacheableMethod, but that can be a follow-up I think.

slasher13’s picture

Status: Needs work » Needs review
FileSize
81.73 KB
19.1 KB

- use Request::isMethodCacheable()
- change constraint in composer.json
- update symfony

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Excellent, I get the same composer changes as in the patch when I run

composer update symfony/*
composer remove ircmaxell/password-compat

This looks ready now. I also updated the version numbers in the change record draft.

I will queue all environments on the testbot to make sure all the test failures are gone now.

alexpott’s picture

+++ b/core/modules/file/config/optional/views.view.files.yml
@@ -565,7 +565,7 @@ display:
-          format_plural_string: "1 place\x03@count places"
+          format_plural_string: !!binary MSBwbGFjZQNAY291bnQgcGxhY2Vz

@@ -1007,7 +1007,7 @@ display:
-          format_plural_string: "1\x03@count"
+          format_plural_string: !!binary MQNAY291bnQ=

This change is quite sad. We're moving from something readable and editable to something that is not. This makes any config change here unreviewable.

dawehner’s picture

This change is quite sad. We're moving from something readable and editable to something that is not. This makes any config change here unreviewable.

I'm wondering whether we could try to use any other list of characters are separator here. We could still convert that to \x03 on runtime.

catch’s picture

alexpott’s picture

+++ b/core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php
@@ -18,7 +18,7 @@ class CommandLineOrUnsafeMethod implements RequestPolicyInterface {
-    if ($this->isCli() || !$request->isMethodSafe()) {
+    if ($this->isCli() || !$request->isMethodCacheable()) {

Does this change have a wider impact on the system? There are plenty of places where we use isMethodSafe() to determine cacheability.

slasher13’s picture

Symfony has reverted the BC break:
https://github.com/symfony/symfony/pull/20602

Next step: Waiting for the next Symfony release. Maybe Symfony 3.2 will be released, too.

So this has no wider impact, but I think we should rename isMethodSafe to the more meaningful isMethodCacheable in a follow-up.

catch’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
90.69 KB
11.51 KB

So we already have #2823687: Use isMethodCacheable() instead of isMethodSafe() when checking for GET/HEAD open. I think we have two choices:

1. Do the isMethodCacheable() change here, that issue then either moves back to 8.2.x or duplicate given the upstream Symfony revert

2. Postpone this issue on a new tagged Symfony release. Again given 3.2 is coming out very soon, I'm keen to get this in so we can start dealing with one release at a time instead of both the major + minor jumps.

So here's a patch that changes all of our usages. There's one or two which might be borderline, but we can revisit those in a follow-up - I think we should just keep our own logic unchanged here.

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good!

I think we should not postpone this yet again. We need Symfony 3 in sooner than later to have enough time to test and get everything ready for 8.3.0.

I'm also annoyed by the YAML !!binary conversion, but we can also deal with that later in #2829919: Either avoid or explicitly test binary encoding in default configuration.

catch’s picture

@alexpott left a comment on the Symfony PR that introduced the change, but initial response doesn't look encouraging for making the behaviour optional, so I agree we should suffer with it for now and address it in #2829919: Either avoid or explicitly test binary encoding in default configuration.

pounard’s picture

My 2 cents: beware with Symfony 3.2, there's a few changes that might cause you trouble, I had problems because of divergence between 3.1 and 3.2 in the DIC generated code, this probably won't affect Drupal, but it might exists other internal changes on pieces of code on which Drupal 8 attempts to plug.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work

There are plenty of places where we use isMethodSafe() to determine cacheability.

Indeed. To be fair, most of them are in the render system, which AFAIK cannot ever be used to respond to TRACE or OPTIONS requests. So sticking with isMethodSafe() should be … safe (heh).

Of course, better to be correct & safe than sorry, so we should update all cache-related usages. Which this patch does. Except it does a few too many:

  1. +++ b/core/modules/rest/src/EventSubscriber/ResourceResponseSubscriber.php
    @@ -96,7 +96,7 @@ public function getResponseFormat(RouteMatchInterface $route_match, Request $req
    -    $acceptable_formats = $request->isMethodSafe() ? $acceptable_request_formats : $acceptable_content_type_formats;
    +    $acceptable_formats = $request->isMethodCacheable() ? $acceptable_request_formats : $acceptable_content_type_formats;
    

    No, these are NOT about cacheability, but about safe (read vs write).

    (3 occurrences here to revert.)

  2. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -178,8 +173,13 @@ public function testOnResponseWithUncacheableResponse($methods, array $supported
    -      $route_requirement_key_format = $request->isMethodSafe() ? '_format' : '_content_type_format';
    ...
    +      $route_requirement_key_format = $request->isMethodCacheable() ? '_format' : '_content_type_format';
    

    Hence this change should also be reverted.

Wim Leers’s picture

  1. +++ b/core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php
    @@ -18,7 +18,7 @@ class CommandLineOrUnsafeMethod implements RequestPolicyInterface {
        * {@inheritdoc}
        */
       public function check(Request $request) {
    -    if ($this->isCli() || !$request->isMethodSafe()) {
    +    if ($this->isCli() || !$request->isMethodCacheable()) {
           return static::DENY;
    

    The class name no longer matches what it does.

  2. +++ b/core/modules/rest/tests/src/Unit/CollectRoutesTest.php
    @@ -141,8 +141,8 @@ public function testRoutesRequirements() {
    -    $this->assertEquals(count($requirements_1), 0, 'First route has no requirement.');
    -    $this->assertEquals(count($requirements_2), 2, 'Views route with rest export had the format and method requirements added.');
    +    $this->assertEquals(0, count($requirements_1), 'First route has no requirement.');
    +    $this->assertEquals(1, count($requirements_2), 'Views route with rest export had the format requirement added.');
    

    Heh, expectations first, yes, great — thanks!

    (Seems out of scope, but is fine.)

  3. +++ b/core/modules/rest/tests/src/Unit/EventSubscriber/ResourceResponseSubscriberTest.php
    @@ -79,11 +79,6 @@ public function providerTestSerialization() {
    -    $parameters = [];
    -    if ($request_format !== FALSE) {
    -      $parameters['_format'] = $request_format;
    -    }
    
    @@ -91,8 +86,13 @@ public function testResponseFormat($methods, array $supported_formats, $request_
    -      $request = Request::create('/rest/test', $method, $parameters, [], [], $request_headers, $request_body);
    ...
    +      $request = Request::create('/rest/test', $method, [], [], [], $request_headers, $request_body);
    +      // \Drupal\Core\StackMiddleware\NegotiationMiddleware normally takes care
    +      // of this so we'll hard code it here.
    +      if ($request_format) {
    +        $request->setRequestFormat($request_format);
    +      }
    

    At first, I was skeptical about this change. But having read #141 + #142 + the related Symfony links, this makes sense.

Wim Leers’s picture

Finally, I'd like to beg core committers to commit #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method before this patch. This patch has a high risk of introducing subtle bugs in our REST API.

slasher13’s picture

Status: Needs work » Needs review
FileSize
93.08 KB
6.73 KB

fixes #248 and #249.1

slasher13’s picture

FileSize
7.9 KB

wrong interdiff

catch’s picture

Status: Needs review » Needs work

@Wim see my note in #248

So here's a patch that changes all of our usages. There's one or two which might be borderline, but we can revisit those in a follow-up - I think we should just keep our own logic unchanged here.

i.e. if we only convert some usages, then while the Symfony API change is in (which it is with the version in this patch), then our logic changes as a by-product. It might get the bug fix, and be actually correct for a while. However when they revert the API change, it'll go back to be just using GET/HEAD anyway, except that we'll be using a deprecated code path (because they're deprecating the usage of isMethodSafe() with no arguments).

So with the version of Symfony we're upgrading to here there is no good way to use isMethodSafe() - and we're going to have to revisit the places that actually want to use it in a follow-up anyway.

Also disagree with renaming CommandLineOrUnsafeMethod here - that's at least an @internal API change, and it's not us who's changed the semantics of unsafemethod it's Symfony - we made no claim to be RFC compatible and nor did they until they made the change.

klausi’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
90.69 KB

Completely agree with catch. Filed the 2 follow ups:
#2830454: Revisit Request::isMethodSafe() usage vs. Request:isMethodCachable()
#2830451: Rename class CommandLineOrUnsafeMethod to CommandLineOrUncacheableMethod

Reuploadind the patch from #244, that's the one we want to commit right now.

catch’s picture

Re-uploading the patch from #244 since it's that one that klausi RTBCed. If there's a solution here that doesn't require waiting for another Symfony release or changing everything again anyway in a follow-up, then that's great, but this patch is the minimal change we can make to update to Symfony 3.1 without introducing new bugs or later unexpected changes on the next patch/minor update afaict.

Wim Leers’s picture

Note I was not asking to rename anything in #249.1. I was only observing that mismatch. I think documenting the intent explicitly in that class is sufficient.

I'm still concerned about introducing the isMethodCacheable() calls in ResourceResponseSubscriber. I'd rather even have that class add its own helper method for that. We need to make it clear that that class is explicitly caring about reading (safe) vs writing (unsafe) methods.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

Hm, the testbot set this to needs work although all patches above are green?

Wim Leers’s picture

Random fail, somebody re-tested #255. See https://www.drupal.org/pift-ci-job/537952 :)

klausi’s picture

Status: Needs work » Reviewed & tested by the community
Wim Leers’s picture

I'd again like to beg to postpone committing this until #2737719: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method lands. Which is blocked on #2822387: Undefined offset 1 in BrowserTestBase::getMethodCaller() (because it caused a regression in 8.3.x that's showing up in #2737719).

slasher13’s picture

Still waiting for 3.1.8

Update to 3.2 to see what's happening.

catch’s picture

I've opened #2831757: Update to Symfony ~3.2 for the ~3.2 upgrade, postponed on this issue.

catch’s picture

Status: Needs work » Needs review
FileSize
90.69 KB

Re-re-uploading the #244 patch now that the REST tests are in. @slasher13 let's continue with 3.2 on the follow-up I posted.

Wim Leers’s picture

You beat me to it.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Woot, green! Back to RTBC.

pounard’s picture

That's a very nice thing, this patch needs to pass, there are a few annoying bugs in the DI component in 2.8 which are solved in 3.x which are getting harder to workaround for contrib as time passes!

pounard’s picture

After applying the patch I got errors such as:

2016/12/01 11:39:59 [error] 3892#3892: *27 FastCGI sent in stderr: "PHP message: Error: Call to undefined method Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher::addSubscriberService() in /var/www/d8-ucms/app/cache/dev/appDevDebugProjectContainer.php on line 4127 #0 /var/www/d8-ucms/lib/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php(274): appDevDebugProjectContainer->getEventDispatcherService()
#1 /var/www/d8-ucms/app/cache/dev/appDevDebugProjectContainer.php(6061): Symfony\Component\DependencyInjection\Container->get('event_dispatche...')
#2 /var/www/d8-ucms/lib/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php(274): appDevDebugProjectContainer->getHttpKernel_BasicService()
#3 /var/www/d8-ucms/app/cache/dev/appDevDebugProjectContainer.php(6150): Symfony\Component\DependencyInjection\Container->get('http_kernel.bas...')
#4 /var/www/d8-ucms/lib/vendor/symfony/symfony/src/Symfony/Component/DependencyInjection/Container.php(274): appDevDebugProjectContainer->getHttpMiddleware_SessionService()
#5 /var" while reading response header from upstream, client: 192.168.57.1, server: d8-ucms.guinevere, request: "GET / HTTP/1.1", upstream: "fastcgi://unix:/var/run/php5-fpm.sock:", host: "d8-ucms.guinevere"

I'm not sure the patch is responsible, because I had a few conflicts applying it atop other patches, but I thought it worth being mentioned.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

So I think the only way we're not going to find further issues is by committing this and seeing what comes back from contrib and early-adopter experience.

Committed a55b8ef and pushed to 8.3.x. Thanks!

  • alexpott committed a55b8ef on 8.3.x
    Issue #2712647 by klausi, catch, slasher13, jibran, Manuel Garcia,...
Wim Leers’s picture

Nice, now on to #2831757: Update to Symfony ~3.2! (Just unpostponed that.)

pounard’s picture

@alexpott nice, thanks!

Wim Leers’s picture

This broke drush.

Latest drush 8.1.8 is not compatible with Symfony 3.1, it fails to rebuild the container, which is what @pounard was already alluding to.

8.3.x before this commit:

wim.leers at acquia in ~/Work/d8 on 8.3.x*
$ drush --version
 Drush Version   :  8.1.8 

wim.leers at acquia in ~/Work/d8 on 8.3.x*
$ git log --oneline -n 1
6e26b86 Issue #2828319 by mradcliffe, Wim Leers, neclimdul: [regression] REST in Drupal 8.2.x does not allow HTTP methods other than GET/PATCH/POST/DELETE: OPTIONS, PUT, et cetera all fail
wim.leers at acquia in ~/Work/d8 on 8.3.x*
$ drush cr
Cache rebuild complete.                                                                                                                                                [ok]

8.3.x after this commit:

wim.leers at acquia in ~/Work/d8 on 8.3.x*
$ git pull
…
wim.leers at acquia in ~/Work/d8 on 8.3.x*
$ git log --oneline -n 1
a55b8ef Issue #2712647 by klausi, catch, slasher13, jibran, Manuel Garcia, alexpott, naveenvalecha, neclimdul, dawehner, Wim Leers, pounard: Update Symfony components to ~3.1
wim.leers at acquia in ~/Work/d8 on 8.3.x*
$ composer install
…
wim.leers at acquia in ~/Work/d8 on 8.3.x*
$ drush cr
PHP Fatal error:  Declaration of Drush\Command\DrushInputAdapter::hasParameterOption($values) must be compatible with Symfony\Component\Console\Input\InputInterface::hasParameterOption($values, $onlyParams = false) in /Users/wim.leers/.composer/vendor/drush/drush/lib/Drush/Command/DrushInputAdapter.php on line 27

Fatal error: Declaration of Drush\Command\DrushInputAdapter::hasParameterOption($values) must be compatible with Symfony\Component\Console\Input\InputInterface::hasParameterOption($values, $onlyParams = false) in /Users/wim.leers/.composer/vendor/drush/drush/lib/Drush/Command/DrushInputAdapter.php on line 27
Drush command terminated abnormally due to an unrecoverable error.                                                                                                     [error]
Error: Declaration of Drush\Command\DrushInputAdapter::hasParameterOption($values) must be compatible with
Symfony\Component\Console\Input\InputInterface::hasParameterOption($values, $onlyParams = false) in
/Users/wim.leers/.composer/vendor/drush/drush/lib/Drush/Command/DrushInputAdapter.php, line 27
pounard’s picture

I wasn't experiencing it with Drush, but yeah I'm not surprised, both drush and Drupal Console will need to be upgraded for 8.3.x.

anavarre’s picture

Filed https://github.com/drush-ops/drush/issues/2474 to get the ball rolling.

jibran’s picture

Updated Drupal branch and version number in CR https://www.drupal.org/node/2743809/revisions/view/10231702/10232029.

klausi’s picture

@Wim: drush cr is working fine for me after updating. Did you forget to run composer install to get the new Symfony dependencies?

yanniboi’s picture

I have found another issue with this.

Drupal\Core\DependencyInjection\YamlFileLoader::parseDefinition() parses yml file definitions (including services) on cache clear.

There is a service property called scope (https://www.drupal.org/docs/8/api/services-and-dependency-injection/stru...), which after having a quick grep is not currently used by any services in core, however by some in contrib.

YamlFileLoader::parseDefinition() calls the setScope() method on the service definition (an instance of Symfony\Component\DependencyInjection\Definition ) which was deprecated in Symfony 2.8 and removed in Symfony 3.0 (https://api.drupal.org/api/drupal/vendor!symfony!dependency-injection!De...)

To recreate this issue, just add 'scope: prototype' to any service in drupal core (eg node.node_route_context in node.services.yml) and clear caches. It will fail with the following error:

PHP Fatal error: Call to undefined method Symfony\Component\DependencyInjection\Definition::setScope() in /var/www/html/a/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php on line 175

jibran’s picture

jibran’s picture

anavarre’s picture

xjm’s picture

Issue tags: +8.3.0 release notes
alexpott’s picture

https://github.com/drush-ops/drush/issues/2474 now links to 2 PRs to fix drush (one for master and one for Drush 8.x)

tduong’s picture

Hello :)

I've tried to run some drush commands and my shell is complaining come incompatible method signature

Fatal error: Declaration of Drupal\Component\DependencyInjection\Container::set() must be compatible with Symfony\Component\DependencyInjection\ContainerInterface::set($id, $service, $scope = self::SCOPE_CONTAINER) in /usr/local/var/www/d8.dev/www/core/lib/Drupal/Component/DependencyInjection/Container.php on line 51

and also about missing method implementations from ContainerInterface

PHP Fatal error: Class Drupal\Component\DependencyInjection\Container contains 5 abstract methods and must therefore be declared abstract or implement the remaining methods (Symfony\Component\DependencyInjection\ContainerInterface::enterScope, Symfony\Component\DependencyInjection\ContainerInterface::leaveScope, Symfony\Component\DependencyInjection\ContainerInterface::addScope, ...) in /usr/local/var/www/d8.dev/www/core/lib/Drupal/Component/DependencyInjection/Container.php on line 601

I've seen this was the last commit that breaks it.
I also cannot access to my DB... Not sure how/if this affects it as well..

Can someone please give me an explanation or tell me if I've assumed it wrong ? ^^'
Thanks everyone :)

tduong’s picture

Oh, I forgot to run composer install...

I'll check again!

alexpott’s picture

Status: Fixed » Needs review

So I've reverted this issue till we have a solution for Drush and Console that means their latest versions can run against multiple Drupal 8 versions ie. 8.2.x and 8.3.x.

To quote @catch

I mean we committed it to find problems and we found some.

I think the only way for those projects to work with this is going to be some almighty autoloader hack and those projects having both Symfony 2.x and 3.x code. This is super super painful.

Setting status back to "needs review" because I want more opinions on solutions to this problem - the patch is rtbc but we have to consider the wider environment and provide recommendations in the CR for how projects like Drush and Console work with this.

  • alexpott committed 79569a7 on 8.3.x
    Revert "Issue #2712647 by klausi, catch, slasher13, jibran, Manuel...
jibran’s picture

I think it is a good idea to revert this but I think we should keep this patch improving by merging #2832119: Remove references of Symfony 2.8 and #2831757: Update to Symfony ~3.2 into this patch. We can also improve the docs. We should also create g.d.o post a week before committing this next time around.

catch’s picture

Let's merge #2832119: Remove references of Symfony 2.8 into here, but I'm not keen on merging #2831757: Update to Symfony ~3.2 in at least until there's an RTBC patch on that issue. Each minor update requires changes on our part that should be kept as reviewable as possible.

Wim Leers’s picture

#290++
#291++

pfrenssen’s picture

You can always use local instances of drush and console instead of global ones. That is a LOT easier than trying to wrangle them to support multiple versions at the same time.

I stopped using global instances of drush already when it stopped supporting D6.

catch’s picture

You can always use local instances of drush and console instead of global ones. That is a LOT easier than trying to wrangle them to support multiple versions at the same time.

While this is true, even doing that makes an 8.2.x to 8.3.x update a bit tricky no?

pfrenssen’s picture

The first step would be to update the code, right? So you use composer to update core to 8.3.x and this will also update drush to a compatible version because Composer will be able to resolve a set of dependencies that work together. Then the next step is to do "drush updb" to get the database in order.

There might be some pitfalls that I'm not thinking of right now, but this workflow seems doable to me. Also from the moment 8.3.0 is out 8.2.x is technically unsupported, so there's no real need for drush/console to support both at the same time with the same version.

webflo’s picture

@pfrenssen is right. A local installation of Drush and console solves the problem, both projects move into this direction. There are a few solutions to avoid the problem.

Drush dispatches all commands from the global installation to the local one, if the Drupal autoloader has a Drush installation. This feature has been added in Version 8.0.3 or 8.0.4 (AFAIR).

I extracted this logic in its own library. This functionality can be shared between Drush and Console. Its already used by Drupal console and i use a small cli tool as global Drush replacement (https://github.com/webflo/drush-shim). Its basically a start for the launcher which @Mosh proposed in Drush 9 Roadmap.

jibran’s picture

FileSize
4.18 KB
94.38 KB
jibran’s picture

Do we have to update these files?

core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
alexpott’s picture

@jibran we probably have to check those files for updates... we have modifications to those classes so we have to be careful not to remove them.

alexpott’s picture

I think @webflo is right the only way to fix this is to install drush and console etc.. as part of Drupal. I.e. make composer responsible for sorting out the problem.

One concern though would be how would site aliases work? How would the local Drush/Console shim get the right codebase for the remote?

andypost’s picture

+++ b/core/lib/Drupal/Component/Serialization/YamlSymfony.php
@@ -18,7 +19,7 @@ public static function encode($data) {
-      return $yaml->dump($data, PHP_INT_MAX, 0, TRUE, FALSE);
+      return $yaml->dump($data, PHP_INT_MAX, 0, SymfonyYaml::DUMP_EXCEPTION_ON_INVALID_TYPE);

Would be great to add new flag Yaml::DUMP_MULTI_LINE_LITERAL_BLOCK

This will help to improve readability of webform export
http://symfony.com/doc/3.1/components/yaml.html#dumping-multi-line-liter...

  • alexpott committed 79569a7 on 8.4.x
    Revert "Issue #2712647 by klausi, catch, slasher13, jibran, Manuel...
  • alexpott committed a55b8ef on 8.4.x
    Issue #2712647 by klausi, catch, slasher13, jibran, Manuel Garcia,...

  • alexpott committed 79569a7 on 8.4.x
    Revert "Issue #2712647 by klausi, catch, slasher13, jibran, Manuel...
  • alexpott committed a55b8ef on 8.4.x
    Issue #2712647 by klausi, catch, slasher13, jibran, Manuel Garcia,...

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Issue tags: -8.3.0 release notes

This was tagged for the release notes before we had to roll it back, so untagging.

xjm’s picture

@catch, @alexpott, and I also discussed this issue two weeks ago in light of the disruption for Drush and contrib, and considered whether it might be better committed to 8.4.x. According to https://symfony.com/roadmap?version=2.8#checker we still have 18 months of full support for Symfony 2.8, so it would be acceptable to update to Symfony 3 for 8.4.x instead. I'm not sure if it is mitigated by #2843828: \Drupal\Core\DrupalKernel::initializeSettings() can result in moving the autoloader position though.

slasher13’s picture

Why not update to Symfony 3.2? https://www.drupal.org/node/2831757

catch’s picture

Issue summary: View changes
slasher13’s picture

Status: Needs review » Needs work
FileSize
92.51 KB

re-roll

slasher13’s picture

slasher13’s picture

Title: Update Symfony components to ~3.1 » Update Symfony components to ~3.2
Issue summary: View changes
FileSize
91.43 KB
17.44 KB

update symfony/* to 3.2.3

pfrenssen’s picture

Phing 2.16.0 which fixes PHP 7.1 compatibility requires Symfony 3.1+

Phing is one of the tools that is commonly used for Continuous Integration. If we postpone this to 8.4.x it will be a very long time for people not being able to use Drupal in combination with Phing on PHP 7.1.

Edit: apparently this has been fixed a few days ago in Phing, and the next release will again be compatible with Symfony 2.8. See https://github.com/phingofficial/phing/issues/658

TJacksonVA’s picture

What is the anticipated timeframe for getting this into the 8.4.x-dev branch? Are there still other dependencies? From what I can gather, doesn't \Drupal\Core\DrupalKernel::initializeSettings() can result in moving the autoloader position fix the drush issue?

The sooner we get this into the 8.4.x-dev branch so that module developers and website developers can get some experience using it and finding any issues, the easier it will be to upgrade to Symfony 3.3.x before Drupal 8.4.x, as Symfony 3.3 is scheduled to be released in May 2017.

moshe weitzman’s picture

I unpublished the CR, [#2743809], as this has not gone in yet.

catch’s picture

We're planning to commit it to 8.4.x around the time of the 8.3.0 release candidate, no actual date set yet or anything but I think that's the general idea.

TJacksonVA’s picture

@catch, many thanks for the update.

slasher13’s picture

update symfony/* to 3.2.4

slasher13’s picture

slasher13’s picture

update symfony/* to 3.2.5

slasher13’s picture

The problem started at https://github.com/symfony/symfony/pull/21564/files

"conflict": {
 +        "phpunit/phpunit": "<4.8.35|<5.4.3,>=5.0"
 +    },

To update to phpunit/phpunit: 4.8.35 I must update sebastian/comparator, too.
Requirement change between 4.8.28 and 4.8.29

slasher13’s picture

update symfony/* to 3.2.6

jibran’s picture

Status: Needs review » Needs work

#302 is still pending. Do we need a follow-up for #300? Do we also need some follow-up issues for https://github.com/drush-ops/drush/pull/2477#issuecomment-264508717 as well?

jibran’s picture

claudiu.cristea’s picture

  1. +++ b/core/modules/file/config/optional/views.view.files.yml
    @@ -565,7 +565,7 @@ display:
    -          format_plural_string: "1 place\x03@count places"
    +          format_plural_string: !!binary MSBwbGFjZQNAY291bnQgcGxhY2Vz
    
    @@ -1007,7 +1007,7 @@ display:
    -          format_plural_string: "1\x03@count"
    +          format_plural_string: !!binary MQNAY291bnQ=
    

    What? EDIT: I read #209 & following

  2. +++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
    @@ -10,6 +10,7 @@
    +use Symfony\Component\Validator\Validator\ValidatorInterface;
    
    @@ -327,7 +328,7 @@ public function testPreSaveRevision() {
    -    $validator = $this->getMock('\Symfony\Component\Validator\ValidatorInterface');
    +    $validator = $this->getMock(ValidatorInterface::class);
    
    @@ -363,7 +364,7 @@ public function testValidate() {
    -    $validator = $this->getMock('\Symfony\Component\Validator\ValidatorInterface');
    +    $validator = $this->getMock(ValidatorInterface::class);
    

    These changes seems unrelated.

slasher13’s picture

Status: Needs work » Needs review
FileSize
92.95 KB
1.68 KB

#302 Should be a follow-up.
#326.2 Done

Create an updated drush pull request:
https://github.com/drush-ops/drush/pull/2672

slasher13’s picture

Status: Needs work » Needs review
FileSize
94.63 KB

re-uploaded patch from #323

#326.2 Isn't unrelated because namespace of ValidatorInterface changed from
'\Symfony\Component\Validator\ValidatorInterface' to
'\Symfony\Component\Validator\Validator\ValidatorInterface'

slasher13’s picture

slasher13’s picture

jibran’s picture

Status: Needs review » Needs work
+++ b/core/composer.json
@@ -43,8 +43,8 @@
+        "symfony/browser-kit": "~3.2",

I think we can remove this line. We are not directly dependent on this package.

slasher13’s picture

Status: Needs work » Needs review
FileSize
404 bytes
91.48 KB

#332 done

jibran’s picture

Thanks, I'll RTBC once https://github.com/drush-ops/drush/pull/2672 is merged.

slasher13’s picture

alexpott’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Needs a reroll.

timmillwood’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
91.33 KB

Re-roll

catch’s picture

Status: Needs review » Reviewed & tested by the community

Re-roll looks fine. I think we should aim to get this in soon after 8.3.0 is released, so that at least we have plenty more opportunities to roll back and re-commit if something comes up.

However it looks like https://github.com/drush-ops/drush/issues/2672 was committed to drush then reverted, and now https://github.com/drush-ops/drush/pull/2692 is open trying to test with a patched 8.4.x.

I've asked on the latter issue to clarify what the current status is. We should not commit this until the drush side is OK, but marking RTBC since there's nothing specific to postpone this on against core.

webflo’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll
webflo’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
46.31 KB
slasher13’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
91.33 KB

re-roll
back to RTBC as #338

mpdonadio’s picture

Synfony 3.2.7 came out this morning. Do we want to keep this or `composer update symfony/*` the patch in #343?

timmillwood’s picture

Status: Reviewed & tested by the community » Needs work

I'd vote for an update to 3.2.7

pounard’s picture

I vote to update to 3.3 :) But yes you're right, it was just released that's a good idea to stick to the higher version until the Drupal branch gets stable itself.

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
91.14 KB
14.57 KB

Here's #343 + `composer update symfony/*`

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

As long as testbot agrees, looks fine.

mpdonadio’s picture

Based on http://symfony.com/blog/symfony-3-2-7-released, I think the fail in #349 is from or related to https://github.com/symfony/symfony/pull/22036. I see what is happening, but not why; I don't know why RedirectResponse doesn't have the 'Date' header, but the SecuredRedirectResponse does.

Side note, the issue mentioned on line 43 of SecuredRedirectResponseTest is closed, and the unsets aren't needed.

mpdonadio’s picture

OK, #349 is an upstream problem.

class Response
{
    public function __construct($content = '', $status = 200, $headers = array())
    {
        $this->headers = new ResponseHeaderBag($headers);
        $this->setContent($content);
        $this->setStatusCode($status);
        $this->setProtocolVersion('1.0');

        // Deprecations
        $class = get_class($this);
        if ($this instanceof \PHPUnit_Framework_MockObject_MockObject || $this instanceof \Prophecy\Doubler\DoubleInterface) {
            $class = get_parent_class($class);
        }
        if (isset(self::$deprecationsTriggered[$class])) {
            // CODE IS RETURNING HERE SO 'DATE' ISN'T BEING SET
            return;
        }

        self::$deprecationsTriggered[$class] = true;
        foreach (self::$deprecatedMethods as $method) {
            $r = new \ReflectionMethod($class, $method);
            if (__CLASS__ !== $r->getDeclaringClass()->getName()) {
                @trigger_error(sprintf('Extending %s::%s() in %s is deprecated since version 3.2 and won\'t be supported anymore in 4.0 as it will be final.', __CLASS__, $method, $class), E_USER_DEPRECATED);
            }
        }

        /* RFC2616 - 14.18 says all Responses need to have a Date */
        if (!$this->headers->has('Date')) {
            $this->setDate(new \DateTime(null, new \DateTimeZone('UTC')));
        }
    }
mpdonadio’s picture

mpdonadio’s picture

Status: Needs work » Needs review
FileSize
91.15 KB

Reroll is b/c #2853300: Standardize fatal error/exception handling: backtrace for all formats, not just HTML, which renamed DefaultExceptionSubscriberTest to FinalExceptionSubscriberTest

The PR mentioned in #352 got merged, but is not in a released version yet. So, we either re-roll on 3.2.6 explicitly or wait on 3.2.8 to do a ~3.2.

Here is is explicitly set to 3.2.6, and `composer update 'symfony/*'`

martin107’s picture

mpdonadio++

thanks for the quick response.

I would like to see this committed as soon as possible.

Above it was stated that we should get this in early in the 8.4.x release window as it will be disruptive.

It would be good to get this in a week or so before the patch commit spree that will be Drupalcon Baltimore - so we can respond to any unforeseen problems.

As Symfony and Drupal8 are adding new features consistent with the semantic version policy D8 - we should be resyncing every 6 months or so
So any reason to delay can be pickup in a future release.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Explicitly on 3.2.6 is OK for me. Opened the follow-up to update to ~3.2 once 3.2.8 is out at #2871253: Update Symfony components to 3.2.8.

Moving this to RTBC.

mpdonadio’s picture

Status: Needs work » Reviewed & tested by the community
catch’s picture

Title: Update Symfony components to ~3.2 » May 2nd: Update Symfony components to ~3.2
Issue summary: View changes

Discussed with alexpott and xjm, and we decided to schedule this for May 2nd just after the sprints. We could also have committed it today maybe but given the number of critical issues in the queue etc. and five months between commit and 8.4.0 being plenty of time to flush out any remaining issues from the upgrade, seems a decent balance.

jibran’s picture

  • catch committed 494271d on 8.4.x
    Issue #2712647 by slasher13, klausi, catch, jibran, Manuel Garcia,...
catch’s picture

Title: May 2nd: Update Symfony components to ~3.2 » Update Symfony components to ~3.2
Status: Reviewed & tested by the community » Fixed

And it's May 2nd!

Committed/pushed to 8.4.x, thanks!

Very hopeful we've flushed out all the regressions with this, but if there are even more, since we're at over 300 comments please open a new issue for follow-ups or reverting, linking to it from this one.

Pages