For the following components the symfony ~2.7 (and ~2.8) version requirement declarations have never been updated and conflict with the ~3.2 and ^3.2 decalarations in drupal/core. See #2867960: Merge Component composer.json files to account for them during build for an attempt to prevent this once and for all.

Changing to ~2.7 || ~3.0 is the hot fix that would keep a composer merge happy. But are these components actually compatible with both symfony 2.7 and 3? Perhaps we can deduce a higher minimum version requirement (~2.8?; ~3.0?) by looking at the git logs. (Or by just looking at all the code for you symfony experts.)

- drupal/core-dependency-injection #2876671: Fix symfony/dependency-injection ~2.8 requirement in drupal/core-dependency-injection
- drupal/core-event-dispatcher #2876675: Fix symfony version requirements in drupal/core-event-dispatcher
- drupal/core-http-foundation #2876703: Fix symfony/http-foundation ~2.7 requirement in drupal/core-http-foundation
- drupal/core-plugin #2876704: Fix symfony/validator ~2.7 requirement in drupal/core-plugin
- drupal/core-serialization #2876707: Fix symfony/yaml ~2.7 requirement in drupal/core-serialization

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Eric_A created an issue. See original summary.

Eric_A’s picture

Title: [META] Fix symfony version requirement for embedded components » [META] Fix symfony version requirements in embedded components
Issue summary: View changes
Eric_A’s picture

Issue summary: View changes
Eric_A’s picture

Issue summary: View changes
Eric_A’s picture

Issue summary: View changes
dawehner’s picture

I believe we should not have an issue for every component but rather fix them all at once. Ideally we would then even have some kind of test, not sure how feasible it is.

Eric_A’s picture

Ideally we would then even have some kind of test, not sure how feasible it is.

Well, there's #2867960: Merge Component composer.json files to account for them during build.

Mile23’s picture

From IS: "For the following components the symfony ~2.7 (and ~2.8) requirements have never been updated and conflict with drupal/core symfony ~3.2 and ^3.2 requirements."

This is not correct. The components in core are mostly the same code between 8.3.x and 8.4.x. We don't have any data about whether they're incompatible or not.

Once we fix the dependencies so they're at least internally consistent in #2867960: Merge Component composer.json files to account for them during build, we can begin to test the actual requirements for these components.

I started work on a travis-ci tester that attempts to run the component tests in isolation from the rest of Drupal core. See it here: https://github.com/paul-m/drupal_component_tester

Eric_A’s picture

@Mile23, ~2.8 is by definition incompatible with ~3.2, according to https://getcomposer.org/doc/articles/versions.md#next-significant-releas...

Eric_A’s picture

jibran’s picture

RE: #6 @dawehner here we go.

jibran’s picture

I'm incline to remove the meta from the title and close the child issues as duplicate.

+++ b/core/lib/Drupal/Component/Diff/composer.json
@@ -6,7 +6,7 @@
-    "drupal/utility": "~8.2"
+    "drupal/core-utility": "~8.4"

We have quite few errors in composer.json.

Mile23’s picture

Status: Needs review » Postponed

This issue really should be postponed on #2867960: Merge Component composer.json files to account for them during build until we get the components into a working state.

Please continue conversing, however. :-)

jibran’s picture

IMO, these issues don't affect each other at all. That fixes the build process this fixes the dependencies.

Mile23’s picture

#2867960: Merge Component composer.json files to account for them during build sets reasonable versions and prevents regressions so everything works well enough to test.

We can't determine minimum or maximum versions for the component dependencies until we're able to test.

catch’s picture

Title: [META] Fix symfony version requirements in embedded components » Fix symfony version requirements in embedded components

Re-titling for non-meta-nature of the issue now, agreed one is fine here.

We're not taking advantage of any Symfony 3 features afaik, but whether we'd actually want to support 2.* at all is a different question.

Eric_A’s picture

Status: Postponed » Needs work

There are about 5 things to think about:

a) Broken dependency declarations between components and components, because of dependency naming mistakes in declarations. (Trivial to fix the dependency names.)
b) Doctrine version declarations in components that conflict with the drupal/core version declaration. (Trivial to fix.)
c) Symfony version declarations in components that conflict with the drupal/core version declarations. (Broken in 8.4 only. Trivial to hotfix if we just change it to a range for the components. Note that in the symfony 2.8 issue @alexpott has noted that we should not break BC if we don't have to.)
d) Getting the symfony version range exactly right for every single component, as wide as possible, no BC break if not needed.
e) Make sure we can't add conflicts ever again. (This will only pass testing if we do either a, b, c, or a, b, d.)

I created this issue with just item d in mind.
I agree with @jibran that this one can start without waiting for item e.
I agree with @Mile23 that a, b, c and e are way more pressing than d.

Eric_A’s picture

Title: Fix symfony version requirements in embedded components » Fix symfony version requirement declarations in embedded components
Eric_A’s picture

Eric_A’s picture

Issue summary: View changes
Mile23’s picture

"Fix symfony version requirement declarations in embedded components"

What I'm getting at here is that we don't know if they're broken, so maybe we don't need to fix them. In #2867960: Merge Component composer.json files to account for them during build we make the 'hot fix' mentioned in the IS and #17 c step. In that issue, we give both 8.4.x and 8.3.x the same set of version constraints: >=2.8 <4.0.0 (or 2.7 in some cases). This allows the components to be functional, if perhaps not absolutely correct on the version constraints.

Once we have that, we can then do more work and figure out the real requirements of a given component. I'm not sure what other projects are good candidates to use these components, but I know the testbot could be a consumer which would give us rapid feedback: #2643110: Have drupalci_testbot require drupal/core-plugin in composer.json It will probably use drupal/core-plugin @stable.

We also have https://github.com/paul-m/drupal_component_tester which will get more attention depending on what next steps we have to undertake. :-)

And finally we need to figure out not only symfony component versions, but core component version dependency as well. For instance, drupal/core-annotation's composer.json in 8.4.x branch says this:

  "require": {
    "php": ">=5.5.9",
    "doctrine/common": "2.5.*",
    "doctrine/annotations": "1.2.*",
    "drupal/core-fileCache": "~8.2",
    "drupal/core-plugin": "~8.2",
    "drupal/core-utility": "~8.2"
  },

As we test things, we might find that these dependencies do not resolve. For instance, if you require drupal/core-annotation ~8.4 then drupal/core-plugin is disallowed because it requires symfony/something ~2.8 instead of ~3.

This sort of interdependency can be considered based on output from a testing script or whatever, or we can just say that all components always require their own branch version of other components in order to not go insane. :-)

Eric_A’s picture

What I'm getting at here is that we don't know if they're broken

Composer will bark if a package requires ~2.8 and another ~3.2, because that cannot be resolved. See also #9.

Mile23’s picture

Composer will bark if a package requires ~2.8 and another ~3.2

Right, but neither Composer nor tests will bark if the component actually needs ~2.8 but we set it to ~2.7 and other packages give us 2.8.* which makes all the tests pass. And it won't bark if we could allow ~2.7 and instead we require ~2.8. Or if the component could run with either ~2.8 or ~3.2 but we require ~3.2 and not ~2.8.

jibran’s picture

Here is a re-roll of #11.

Status: Needs review » Needs work

The last submitted patch, 24: meta_fix_symfony-2876669-11.patch, failed testing. View results

Yogesh Pawar’s picture

Assigned: Unassigned » Yogesh Pawar
Yogesh Pawar’s picture

Assigned: Yogesh Pawar » Unassigned
Status: Needs work » Needs review
FileSize
8.04 KB

Updated patch because #24 failed to apply on 8.4.x branch.

Status: Needs review » Needs work

The last submitted patch, 27: fix_symfony_version-2876669-27.patch, failed testing. View results

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

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

Mile23’s picture

Issue tags: +composer
Mile23’s picture

#2876725: Fix trivial core component composer bugs before 8.4.0 is in now, so we can actually test the version constraints.

Updated my little travis test thing here: https://travis-ci.org/paul-m/drupal_component_tester

It shows us that we still have a symfony version error:

PHP 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 /home/travis/build/paul-m/drupal_component_tester/vendor/drupal/core-dependency-injection/Container.php on line 47

I intuit that this is a version constraint error in drupal/core-dependency-injection, but I could be wrong.

Also the test up to that point is a little bit of a false positive, because each component has requirements like "drupal/core-utility": "^8.2", which allows 8.4.0-dev to resolve drupal/core-utility 8.3.7 (the most recent stable).

We should figure out if we can require self.version for those.

Mile23’s picture

Worked on drupal/core-dependency-injection here: #2876671-8: Fix symfony/dependency-injection ~2.8 requirement in drupal/core-dependency-injection

So very tedious. We need a way to do this automatically.

Version: 8.5.x-dev » 8.6.x-dev

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

jibran’s picture

Issue tags: +Needs reroll
Mile23’s picture

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

I updated my components tester.

You can see the code here: https://github.com/paul-m/drupal_component_tester

You can see the results here: https://travis-ci.org/paul-m/drupal_component_tester

You can clone the project and run it locally if you emulate the setup steps in the .travis.yml file.

It's tricky to test patched components in isolation while also testing their version constraints on other components. That is, for a real test of version constraints, you don't want to depend on an unpatched version of another component.

So my test currently avoids testing drupal/core-discovery, drupal/core-plugin, drupal/core-render, and drupal/core-utility, because they fail for reasons I don't understand. If someone can repro those fails and improve the patch, then please go for it.

Otherwise we might need to do this in multiple phases.

drupal/core-plugin already has an issue, because the test depends on a core module: #2661542: Isolate Drupal\Tests\Component\Plugin\DefaultFactoryTest from core test module

Here's the patch I came up with that allows the travis test to pass.

I did a shortcut of requiring @stable for some of the components. This might cause the Composer phase of the testbot to complain since #2867960: Merge Component composer.json files to account for them during build is in.

Let's find out together.

All the testing applies to 8.5.x. The patch applies to 8.6.x and we'll see if it breaks something, but probably not. This issue should really be 8.5.x, but I won't argue too hard.

Mile23’s picture

Title: Fix symfony version requirement declarations in embedded components » Fix dependency version requirement declarations in components
FileSize
5.74 KB

Some more refined changes to the components' composer.json files.

The patch here matches the patch at https://github.com/paul-m/drupal_component_tester/blob/master/patch/fixe...

You can see that I ran into problems with testing on PHP 7.2 for the same reasons as #2927806: Use PHPUnit 6 for testing when PHP version >= 7.2 but trying a post-install script switch to PHPUnit 6 didn't help me for reasons I don't quite understand.

We get a pass for PHP 5.5-7.1 however.

Unless anyone has any better ways to test version constraints under PHP 7.2, this is the best I can offer with finite time.

Mixologic’s picture

To just get the 7.2. tests to run properly, would adding a `composer require phpunit/phpunit:^6` after the composer install step to the travis.yml file for that one build definition help?

Mile23’s picture

The composer install step in .travis.yml is for the testing tool, which doesn't even use PHPUnit.

The tool then uses exec() to call on composer to do an update: https://github.com/paul-m/drupal_component_tester/blob/master/tester.php...

Mile23’s picture

Mile23’s picture

OK, the travis test is happy with PHP 7.2 now. Ask me in slack or something if you want to know how. :-)

Here's the magic test run against Drupal 8.5.x: https://travis-ci.org/paul-m/drupal_component_tester/builds/335891599

And here it is for Drupal 8.6.x: https://travis-ci.org/paul-m/drupal_component_tester/builds/335900474

drupal/core-event-dispatcher has tests that subclass tests from symfony/event-dispatcher, which is not all that great: #2895487: ContainerAwareEventDispatcherTest depends on Symfony tests

But that dependency then creates the minimum constraint for drupal/core-event-dispatcher, because it has to run under PHPUnit 6, and Symfony's test doesn't, for versions of Symfony lower than 3.4. (Previous versions use \PHPUnit_Framework_Testcase. This changes in 3.3, but we still see the error this causes during the test run against 3.3.)

If we break that dependency in #2895487: ContainerAwareEventDispatcherTest depends on Symfony tests it's very likely that we can specify an older minimum version. If anyone wants to keep an eye on that. :-)

So here's our latest greatest patch. Testing against different PHP versions and also for Drupal 8.5.x.

dawehner’s picture

@mile23
Automating this bit is a serious form of art. This is quite nice!

+++ b/core/lib/Drupal/Component/Annotation/composer.json
@@ -7,9 +7,9 @@
-    "drupal/core-plugin": "^8.2",
+    "drupal/core-plugin": "^8.5",

+++ b/core/lib/Drupal/Component/Discovery/composer.json
@@ -7,7 +7,12 @@
+    "drupal/core-filesystem": "^8.2",
+    "drupal/core-serialization": "^8.5",

So this is a patch against Drupal 8.6.x, shouldn't the dependency take that into account? Maybe I'm just confused ...

Mile23’s picture

+++ b/core/lib/Drupal/Component/Annotation/composer.json
@@ -7,9 +7,9 @@
-    "drupal/core-plugin": "^8.2",
+    "drupal/core-plugin": "^8.5",

So this is a patch against Drupal 8.6.x, shouldn't the dependency take that into account?

The version constraints for components should be wide. Ideally, wider than only the version of core where they live.

That way someone using them stands a better chance of being able to use them given their other composer.json requirements.

Determining the actual wide constraints (rather than guessing) is the point of the travis test.

So in the example of Annotation's dependency on Plugin, let's prove it using git log. :-)

$ git log --pretty=oneline 8.4.x..8.5.x core/lib/Drupal/Component/Plugin/

That shows us that between 8.4.x and 8.5.x we had a ton of coding standards issues (which shouldn't matter here), but also #2867960: Merge Component composer.json files to account for them during build which changed the composer.json dependencies allowing the components to be used at all. So that's why we need ^8.5 for drupal/core-annotation. We could probably specify an older version, like ^8.1 or whatever, but we can't prove that it would work, unless someone has a time machine.

Doing git log for 8.5.x..8.6.x we see that the only thing that changed in drupal/core-plugin was a fix to the licensing field in composer.json, so we don't specifically need 8.6.x for any code changes it introduced. Because it didn't introduce any. :-)

If we change something else in the Plugin component before the 8.6.0 release, we might need to change the requirements for drupal/core-annotation, but for now the patch above is a best faith effort. If we could truly turn this test into a CI step it would make it easier to catch that kind of change. #2867960: Merge Component composer.json files to account for them during build gives us a little bit of confidence, but it's not thorough.

I get that this is hard to review. :-) But the sooner we get it committed (especially to 8.5.x if possible) the better.

Mile23’s picture

I neglected to stress that drupal/core-plugin itself can't be tested in this way because it depends on a module: #2661542: Isolate Drupal\Tests\Component\Plugin\DefaultFactoryTest from core test module

Mile23’s picture

Mile23’s picture

Mile23’s picture

With #2661542: Isolate Drupal\Tests\Component\Plugin\DefaultFactoryTest from core test module in, I updated the component tester to run the plugin tests. Outlook is good: https://travis-ci.org/paul-m/drupal_component_tester/builds/345500113

Still using the patch from #40.