Fix dependency version requirement declarations in components

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.

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

RE: #6 @dawehner here we go.

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.

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. :-)

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

#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.

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.

"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. :-)

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.

Here is a re-roll of #11.

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

#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.

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.

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.

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.

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?

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...

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: [Symfony 4] 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: [Symfony 4] 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.

@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 ...

+++ 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.

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

#2661542: Isolate Drupal\Tests\Component\Plugin\DefaultFactoryTest from core test module updated, could use a review. :-)

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.

#2935193: Fix broken exceptions in Gettext component added a usage of vfsStream to a test, so now we have to update the patch here to reflect that.

Github: https://github.com/paul-m/drupal_component_tester/commit/669b6a66a3b0cbe...

Travis test run: https://travis-ci.org/paul-m/drupal_component_tester/builds/360767187

Restarted testbot after fix.

If we're adding dev dependencies then we also need to add PHPUnit as a dev dependency. But the problem also is that there might side effects from core/tests/bootstrap.php that component tests rely on.

I neglected to remember that I had commented out the Utility tests and never gotten back to them. So I'm working on that.

So far only Utility has needed anything from core's test bootstrap file, and that's to load the Assertion component and say Handle::register(): https://github.com/paul-m/drupal_component_tester/blob/master/workspace/...

Travis: https://travis-ci.org/paul-m/drupal_component_tester/builds/363372119

Forgot the new patch, which adds Assertion as a dev dependency of Utility.

If we're adding dev dependencies then we also need to add PHPUnit as a dev dependency. But the problem also is that there might side effects from core/tests/bootstrap.php that component tests rely on.

Reading through it I'm wondering whether we could have a custom testsuite for our component tests, which uses a way more minimal bootstrap.php

Couple related issues.

Re making a testsuite: #2943856-11: [meta] Reorganize Components so they are testable in isolation and subsequent comments.

Reroll after #2849669: Fix \Drupal\Component\Utility\Unicode() because of the Symfony mbstring polyfill + Added PHPUnit after #50.

Current Travis build is mostly happy: https://travis-ci.org/paul-m/drupal_component_tester/builds/381191450

Except for the PHP 5.5 version which fails looking like this:

2) Drupal\Tests\Component\Utility\UnicodeTest::testStrtolower with data set #1 ('FrançAIS is ÜBER-åwesome', 'français is über-åwesome')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'français is über-åwesome'
+'fran��ais is ��ber-��wesome'

See it here (scroll all the way down): https://travis-ci.org/paul-m/drupal_component_tester/jobs/381191451

Note that's the dependency that we changed in #2849669: Fix \Drupal\Component\Utility\Unicode() because of the Symfony mbstring polyfill

Contemplating how to turn this into a drupalci.yml type build, which will be needlessly complex before #2943856: [meta] Reorganize Components so they are testable in isolation

Back to NR after testbot fix.

Updated the travis test to use core 8.8.x branch.

drupal/core-utilities now requires egulias/email-validator after #2755401: Upgrade EmailValidator to 2.x. That issue didn't put it in the component composer.json. This demonstrates that we need an isolated component dependency bounds test.

NW after #3039611: Update core PHP dependencies for 8.8.x.

Bit of a reroll after #3053363: Remove support for PHP 5 in Drupal 8.8

Travis build here: https://travis-ci.org/paul-m/drupal_component_tester/builds/544370126

In other news, this test could be in core (instead of Travis-CI) if we had #3031379: Add a new test type to do real update testing

Do we need to accommodate the new components in this patch?

Reroll.

One more related added #3179197: Drupal components depend on ^8.8 even in their Drupal 9 version

#3272110: Drupal 9 and 10's Drupal\Component composer.json files are totally out of date is in, so therefore component dependencies will be automagically modified to reflect those of core.