Problem/Motivation

This might be a critical issue, but I'm marking it as major because I'm not sure.

I was trying to work on this thing: #2242947: Integrate Symfony Console component to natively support command line operations but ran into a bunch of trouble with dependencies.

Originally, this issue came about because dependencies couldn't be rebuilt using Composer.

#2366043: Upgrade to Symfony 2.6.0-beta1 has allowed rebuilding the dependencies, but it's still best practice to avoid depending on specific commits.

An easy way to move towards best practices with Composer is to type composer validate. Currently, Composer says the following about Drupal's composer.json file:

./composer.json is valid, but with a few warnings
See http://getcomposer.org/doc/04-schema.md for details on the schema
require.doctrine/common : unbound version constraints (dev-master#a45d110f71c323e29f41eb0696fa230e3fa1b1b5) should be avoided
require.phpunit/phpunit-mock-objects : unbound version constraints (dev-master#e60bb929c50ae4237aaf680a4f6773f4ee17f0a2) should be avoided

Issues which gave us these specific dependency needs.

Original issue summary follows...

It turns out the current composer.json can't be rebuilt.

Try it: Delete composer.lock and core/vendor/. Then type composer update. Build will fail.

This is because our dependencies are locked to specific git commits, rather than versions, and those commits can't be resolved in the dependency tree. Specifically, phpunit 4.1 needs a version of phpunit-mock-objects that can't be supplied.

Changing these to reasonable minor versions allows Composer to fill our needs, and also passes all unit tests.

Proposed resolution

Change composer.json to allow Drupal 8 to be rebuilt.

Remaining tasks

  • Change the composer.json file, update and make a patch.
  • Allow the testbot to have a go.
  • Review changes.
  • Commit changes.

Also: Maybe add a Composer rebuild to the testbot regimen.

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Unfrozen changes Unfrozen because this change creates stability for Drupal, and also helps other projects, such as the Drupal console, to set up their continuous integration systems.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23’s picture

Status: Active » Needs review
FileSize
902 bytes
660.85 KB

Two patches. One is just the changes to composer.json, and the other is the whole enchilada.

The changes here pass unit tests with ./vendor/bin/phpunit, but let's see what the testbot says.

Mile23’s picture

Issue summary: View changes
chx’s picture

Status: Needs review » Needs work

I asked Mile23 to please run git blame to find out when and why the specific commits were added and whether it's safe to change adding it's extremely likely they were added because we needed fixes for specific issues and if the branch you prescribe include those then we are good. He says he will when the bot passes. So, this either needs work because the tests fail or it needs work because the tests pass and then further research is needed.

Mile23’s picture

Yup. Work for everyone always.

Mile23’s picture

Issue summary: View changes
Mile23’s picture

Issue summary: View changes

The last submitted patch, 1: 2375997_1.patch, failed testing.

Mile23’s picture

Status: Needs work » Needs review
FileSize
626.61 KB
449 bytes

The testbot says Symfony/Yaml is the only problem, which is born out by the discussion in #2234277: Composer update (includes security fixes). Creating a requirement alias solves the problem of being able to install dependencies through composer, as well as keeping the specific commit requirement for Symfony/Yaml.

It's pretty clear that in an ideal world we'd be able to pin Symfony/Yaml to a version instead of a commit.

dawehner’s picture

Note: #2366043: Upgrade to Symfony 2.6.0-beta1 seems to also fix that, it works perfectly and also has the performance optimization.

hussainweb’s picture

@chx, @Mile23: The reason the specific commit was added was to bring in a fix for how the Yaml parser worked with floats and integers. This was originally done in #2350917: Update Symfony YAML library to support whole number floats but it was an invalid commit hash (which pulled in the fix anyway). The commit hash was later fixed in #2234277: Composer update (includes security fixes). See comments #2234277-39: Composer update (includes security fixes) onwards.

And @dawehner is right, the commit is present in 2.6.0-beta1, but not in 2.5.6, which was released after the commit. It appears that we cannot use 2.5.* anymore.

Mile23’s picture

Thanks @dawehner and @hussainweb.

#2366043: Upgrade to Symfony 2.6.0-beta1 will eventually fix the Yaml part, but doctrine/common and phpunit/phpunit-mock-objects still need some love.

The patch here would fix a problem until Symfony does its release to 2.6 and/or 2.7.

tstoeckler’s picture

Status: Needs review » Needs work

Hmm.. I was not able to recreate the Drupal dependencies with the symfony/yaml alias. It seems this may not be working per https://github.com/composer/composer/issues/3374 Note that I am using latest Composer. @Mile23, if this worked for you, which Composer version are you using?

I also had this problem and fixed it by simply requiring symfony/yaml in 2.6.0-beta1, which includes the required fix. Updating only the YAML component to 2.6 shouldn't cause any problems. Although using an alias would be the "proper" fix here, nice idea. I hadn't thought of that.

Mile23’s picture

Aliases aren't really a proper fix either. They fake out Composer into allowing a version as a dependency. That's why I chose to alias to 2.5.0, because all our other Symfony components are at 2.5.*, and 2.5.0 would be the lowest bugfix number, most likely to break later. :-)

I'm working with the latest composer... just do composer selfupdate and magically get it.

It might be a little confusing from the patches and interdiffs, so here's the whole modified composer.json. Copy it to compoer.json, do a rm composer.lock and a rm -rf core/vendor and then composer install.

{
  "name": "drupal/drupal",
  "description": "Drupal is an open source content management platform powering millions of websites and applications.",
  "type": "drupal-core",
  "license": "GPL-2.0+",
  "require": {
    "php": ">=5.4.5",
    "sdboyer/gliph": "0.1.*",
    "symfony/class-loader": "2.5.*",
    "symfony/css-selector": "2.5.*",
    "symfony/dependency-injection": "2.5.*",
    "symfony/event-dispatcher": "2.5.*",
    "symfony/http-foundation": "2.5.*",
    "symfony/http-kernel": "2.5.*",
    "symfony/routing": "2.5.*",
    "symfony/serializer": "2.5.*",
    "symfony/validator": "2.5.*",
    "symfony/yaml": "dev-master#499f7d7aa96747ad97940089bd7a1fb24ad8182a as 2.5.0",
    "twig/twig": "1.16.*",
    "doctrine/common": "2.4.*",
    "doctrine/annotations": "1.2.*",
    "guzzlehttp/guzzle": "~5.0",
    "symfony-cmf/routing": "1.3.*",
    "easyrdf/easyrdf": "0.8.*",
    "phpunit/phpunit": "4.1.*",
    "phpunit/phpunit-mock-objects": "2.1.*",
    "zendframework/zend-feed": "2.2.*",
    "mikey179/vfsStream": "1.*",
    "stack/builder": "1.0.*",
    "egulias/email-validator": "1.2.*"
  },
  "autoload": {
    "psr-4": {
      "Drupal\\Core\\": "core/lib/Drupal/Core",
      "Drupal\\Component\\": "core/lib/Drupal/Component",
      "Drupal\\Driver\\": "drivers/lib/Drupal/Driver"
    },
    "files": [
      "core/lib/Drupal.php"
    ]
  },
  "config": {
    "vendor-dir": "core/vendor",
    "preferred-install": "dist",
    "autoloader-suffix": "Drupal8"
  }
}
tstoeckler’s picture

The 2.6 issue just got in, so let's just fix the other two offenders here.

Mile23’s picture

Status: Needs work » Needs review
FileSize
172.21 KB
769 bytes
Mile23’s picture

Title: Drupal 8 dependencies can't be rebuilt with current composer.json » Avoid tying Drupal 8's composer.json to specific package commits.
Priority: Major » Normal

Changing title to reflect what's really going on here.

Drupal's dependencies can be rebuilt after #2366043: Upgrade to Symfony 2.6.0-beta1, so this just cleans up the remaining commit-centric dependencies.

Status: Needs review » Needs work

The last submitted patch, 15: 2375997_15.patch, failed testing.

isntall queued 15: 2375997_15.patch for re-testing.

Mile23’s picture

Status: Needs work » Needs review
Mile23’s picture

Issue summary: View changes
Mile23’s picture

Issue summary: View changes
dawehner’s picture

Is there a reason we don't go straight to phpunit/phpunit-mock-objects 2.4 ?

Mile23’s picture

Feel free to add a patch for it.

Mile23’s picture

hussainweb’s picture

I made the same changes to composer.json and ran composer update only for those two packages. The output:

vagrant@d8:/var/www/d8task-[git 8.0.x] $ composer update doctrine/common phpunit/phpunit-mock-objects
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Removing doctrine/common (dev-master a45d110)
  - Installing doctrine/common (v2.4.2)
    Downloading: 100%

  - Removing phpunit/phpunit-mock-objects (dev-master e60bb92)
  - Installing phpunit/phpunit-mock-objects (2.1.5)
    Downloading: 100%

Writing lock file
Generating autoload files
omers’s picture

Status: Needs review » Reviewed & tested by the community

Following the instructions on #25 work's fine :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 25: avoid_tying_drupal_8_s-2375997-25.patch, failed testing.

daffie’s picture

Issue tags: +Needs reroll
omers’s picture

Status: Needs work » Needs review
FileSize
5.45 KB
79.73 KB

I made a patch changing the composer.json and composer.lock, my question is ... that's ok or we need upload the files in core/vendors ?

shravan sonkar’s picture

I am working on it

hussainweb’s picture

I started from scratch again and updated composer.json to load doctrine/common version 2.4.* and phpunit-mock-objects versions 2.3.*. We have already updated phpunit to 2.4 release and it appears that the corresponding phpunit-mock-objects release is 2.4.* but that is currently not stable. I think this should work.

Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Removing phpunit/phpunit-mock-objects (dev-master e60bb92)
  - Installing phpunit/phpunit-mock-objects (2.3.0)
    Downloading: 100%

  - Removing doctrine/common (dev-master a45d110)
  - Installing doctrine/common (v2.4.2)
    Downloading: 100%

Writing lock file
Generating autoload files
tstoeckler’s picture

Status: Needs review » Needs work

I think it should be "doctrine/common": "^2.4.2" as that is the first release that contained the previously referenced commit.

The phpunit-mock-objects line can be removed completely, as far as I'm aware, because phpunit already requires an adequate version of phpunit-mock-objects.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
904 bytes
117.26 KB

@tstoeckler: I changed it to ~2.4.2 which is equivalent to 2.4.* from earlier and still enforces the minimum version as 2,4,2. Using ^2.4.2 would immediately pull in 2.5.* and later until 3.0 as well.

Let me see if it holds true for phpunit-mock-objects. It might have been introduced due to the specific commit being referenced earlier.

Status: Needs review » Needs work

The last submitted patch, 33: avoid_tying_drupal_8_s-2375997-33.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review

It looks like this is a random error. It tried to copy a file for migration and couldn't find it.

Mile23’s picture

I changed it to ~2.4.2 which is equivalent to 2.4.* from earlier and still enforces the minimum version as 2,4,2. Using ^2.4.2 would immediately pull in 2.5.* and later until 3.0 as well.

Are you sure about that?

~2.4.2 will pull in any version after 2.4.2. (So 2.5, 3.0, and 23.0.)

2.4.* will pull in any version that starts with 2.4. (So 2.4.x, but not 2.5 or 3.0 or 23.0.)

^2.4. will pull in any version up to the next semantic release, (So 2.4.x and 2.9.x , but not 3.0 or 23.0.)

https://getcomposer.org/doc/01-basic-usage.md#package-versions

Also, we want to be able to use composer update without specifying a package, so that should be the test.

hussainweb’s picture

@Mile23: Yes, I'm quite sure. ~2.4.2 will only pull in 2.4.* upwards of 2.4.2. It won't pull in 2.5.* or anything later.

See the exact section on that page for tilde operator. You are right about the other operators.

The ~ operator is best explained by example: ~1.2 is equivalent to >=1.2 <2.0.0, while ~1.2.3 is equivalent to >=1.2.3 <1.3.0

hussainweb’s picture

@Mile23: As to your second comment:

Also, we want to be able to use composer update without specifying a package, so that should be the test.

I am not sure what you mean exactly here. Are you saying that we shouldn't specify package names when running composer update? That still holds true. I only specified it to update only these two packages and not touch other packages as they would be out of scope.

tstoeckler’s picture

FileSize
730 bytes
89.19 KB

Here we go. Here is proof that it is in fact OK to remove the explicit phpunit/phpunit-mock-objects declaration:

tobias@Tobias-ThinkPad-T420:/var/www/sites/drupal-8/drupal-8/drupal/core$ composer update doctrine/common
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Removing doctrine/common (dev-master a45d110)
  - Installing doctrine/common (v2.4.2)
    Loading from cache

  - Removing phpunit/phpunit-mock-objects (dev-master e60bb92)
  - Installing phpunit/phpunit-mock-objects (2.3.0)
    Loading from cache

Writing lock file
Generating autoload files

Would be nice to get this fixed, as with the beta 6 these two packages are now the only ones that have to be cloned and thus are the only ones that prevent a super fast composer install.

hussainweb’s picture

@tstoeckler: I am not sure if this counts as proof. This might work and even the tests pass as phpunit depends on phpunit-mock-objects and will get it in anyway.

I think the real reason of putting it in here is if we are directly using the package, and I am not sure if we are. AFAICT, we are only using mocking by calling getMock() or variants and I am not sure if that really counts as using this package.

tstoeckler’s picture

Ahh I see your point. No, we are not using the package specifically though. See #2287139: Various serialization test failures after updating to php 5.4.29 for where that line was introduced. It was used to specifically pin the already required package to a newer version. Now that the version is implicitly required anyway, it's obsolete.

Mile23’s picture

Re: phpunit-mock-objects: It's a dependency of phpunit so it will be included. If we needed a specific package commit or had specific version requirements for that package, then we should specify it. Since we don't, we shouldn't. :-)

This is why we *should* have a CI step (daily? weekly?) where we do composer update and then run all the tests, to see if our requirements break our code or vice-versa. Or #2315537: Install composer dependencies before running tests where we pull down all the requirements every time anyway.

tstoeckler’s picture

@Mile23. Care to RTBC, then? ;-P

I know you posted a patch here earlier, but in this case the patch rolling is mostly done by Composer anyway, and we now agree (?!) on the exact version constraints, so I think it would be OK in this instance.

Mile23’s picture

Status: Needs review » Needs work

OK. :-)

So the patch applies and the tests pass, so the patch works. I can RTBC this patch to composer.json because it's doing the right thing.

But, like I mentioned, you might apply the patch and then just do composer update, and then patch *that* for the testbot to evaluate. Then we can know if our version requirements do or don't break Drupal.

Its my intuition that the given patch only did composer update phpunit-mock-object or whatever, because when I did a non-specific composer update, I got a screen full of updates to just about every package:

$ composer update
Loading composer repositories with package information
Updating dependencies (including require-dev)
  - Removing symfony/class-loader (v2.6.1)
  - Installing symfony/class-loader (v2.6.3)
    Loading from cache

  - Removing symfony/css-selector (v2.6.1)
  - Installing symfony/css-selector (v2.6.3)
    Loading from cache

  - Removing symfony/dependency-injection (v2.6.1)
  - Installing symfony/dependency-injection (v2.6.3)
    Loading from cache

  - Removing symfony/event-dispatcher (v2.6.1)
  - Installing symfony/event-dispatcher (v2.6.3)
    Loading from cache

  - Removing symfony/routing (v2.6.1)
  - Installing symfony/routing (v2.6.3)
    Loading from cache

  - Removing symfony/serializer (v2.6.1)
  - Installing symfony/serializer (v2.6.3)
    Loading from cache

  - Removing symfony/translation (v2.6.1)
  - Installing symfony/translation (v2.6.3)
    Loading from cache

  - Removing symfony/validator (v2.6.1)
  - Installing symfony/validator (v2.6.3)
    Loading from cache

  - Removing symfony/process (v2.6.1)
  - Installing symfony/process (v2.6.3)
    Loading from cache

  - Removing twig/twig (v1.16.2)
  - Installing twig/twig (v1.16.3)
    Loading from cache

  - Removing doctrine/lexer (v1.0)
  - Installing doctrine/lexer (v1.0.1)
    Loading from cache

  - Removing doctrine/annotations (v1.2.1)
  - Installing doctrine/annotations (v1.2.3)
    Loading from cache

  - Removing react/promise (v2.1.0)
  - Installing react/promise (v2.2.0)
    Loading from cache

  - Removing guzzlehttp/ringphp (1.0.3)
  - Installing guzzlehttp/ringphp (1.0.5)
    Loading from cache

  - Removing guzzlehttp/guzzle (5.0.3)
  - Installing guzzlehttp/guzzle (5.2.0)
    Downloading: 100%         

  - Removing sebastian/version (1.0.3)
  - Installing sebastian/version (1.0.4)
    Loading from cache

  - Installing sebastian/recursion-context (1.0.0)
    Loading from cache

  - Removing sebastian/exporter (1.0.2)
  - Installing sebastian/exporter (1.2.0)
    Downloading: 100%         

  - Removing sebastian/environment (1.2.0)
  - Installing sebastian/environment (1.2.1)
    Loading from cache

  - Removing sebastian/comparator (1.0.1)
  - Installing sebastian/comparator (1.1.1)
    Downloading: 100%         

  - Removing symfony/yaml (v2.6.1)
  - Installing symfony/yaml (v2.6.3)
    Loading from cache

  - Removing phpunit/php-token-stream (1.3.0)
  - Installing phpunit/php-token-stream (1.4.0)
    Loading from cache

  - Removing phpunit/php-code-coverage (2.0.11)
  - Installing phpunit/php-code-coverage (2.0.15)
    Loading from cache

  - Removing phpunit/phpunit (4.4.2)
  - Installing phpunit/phpunit (4.4.5)
    Downloading: 100%         

  - Removing zendframework/zend-stdlib (2.3.3)
  - Installing zendframework/zend-stdlib (2.3.4)
    Loading from cache

  - Removing zendframework/zend-escaper (2.3.3)
  - Installing zendframework/zend-escaper (2.3.4)
    Loading from cache

  - Removing zendframework/zend-feed (2.3.3)
  - Installing zendframework/zend-feed (2.3.4)
    Loading from cache

  - Removing symfony/http-foundation (v2.6.1)
  - Installing symfony/http-foundation (v2.6.3)
    Loading from cache

  - Removing symfony/debug (v2.6.1)
  - Installing symfony/debug (v2.6.3)
    Loading from cache

  - Removing symfony/http-kernel (v2.6.1)
  - Installing symfony/http-kernel (v2.6.3)
    Loading from cache

  - Removing stack/builder (v1.0.2)
  - Installing stack/builder (v1.0.3)
    Loading from cache

  - Removing egulias/email-validator (1.2.5)
  - Installing egulias/email-validator (1.2.7)
    Loading from cache

  - Removing doctrine/cache (v1.3.1)
  - Installing doctrine/cache (v1.4.0)
    Loading from cache

  - Removing doctrine/inflector (v1.0)
  - Installing doctrine/inflector (v1.0.1)
    Loading from cache

Writing lock file
Generating autoload files
tstoeckler’s picture

@Mile23: Yes I just did composer update doctrine/common. We generally try to keep issues focused on their scope as much as possible. And the focus of this issue is not to update our dependencies.

I personally have nothing against this (in fact I wholeheartedly endorse it), but I fear that committers will see this differently. That's why I provided a minimal patch. Then we can update dependencies in a separate issue.

Mile23’s picture

Status: Needs work » Reviewed & tested by the community

Really? Hmm.

Well, in that case, I'll RTBC and let the maintainer wrestle with this. :-)

hussainweb’s picture

As far as Symfony upgrades are concerned, there is an issue for that (TM): #2414235: Upgrade to Symfony 2.6.4

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 61cb3e3 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation to the issue summary.

  • alexpott committed 61cb3e3 on 8.0.x
    Issue #2375997 by Mile23, hussainweb, tstoeckler, omers: Avoid tying...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.