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.
- #2234277: Composer update (includes security fixes)
- #2173719: Remove the fork of doctrine/common
- #2287139: Various serialization test failures after updating to php 5.4.29
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
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. |
---|
Comment | File | Size | Author |
---|---|---|---|
#40 | 2375997-40-for-realz.patch | 89.19 KB | tstoeckler |
#40 | 2375997-40-for-review-do-not-test.patch | 730 bytes | tstoeckler |
Comments
Comment #1
Mile23Two 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.Comment #2
Mile23Comment #3
chx CreditAttribution: chx commentedI 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.
Comment #4
Mile23Yup. Work for everyone always.
Comment #5
Mile23Comment #6
Mile23Comment #8
Mile23The 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.
Comment #9
dawehnerNote: #2366043: Upgrade to Symfony 2.6.0-beta1 seems to also fix that, it works perfectly and also has the performance optimization.
Comment #10
hussainweb@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.
Comment #11
Mile23Thanks @dawehner and @hussainweb.
#2366043: Upgrade to Symfony 2.6.0-beta1 will eventually fix the Yaml part, but
doctrine/common
andphpunit/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.
Comment #12
tstoecklerHmm.. 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.
Comment #13
Mile23Aliases 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 arm composer.lock
and arm -rf core/vendor
and thencomposer install
.Comment #14
tstoecklerThe 2.6 issue just got in, so let's just fix the other two offenders here.
Comment #15
Mile23Re-roll after #2366043: Upgrade to Symfony 2.6.0-beta1
Comment #16
Mile23Changing 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.
Comment #19
Mile23Comment #20
Mile23Comment #21
Mile23Comment #22
dawehnerIs there a reason we don't go straight to phpunit/phpunit-mock-objects 2.4 ?
Comment #23
Mile23Feel free to add a patch for it.
Comment #24
Mile23Comment #25
hussainwebI made the same changes to composer.json and ran composer update only for those two packages. The output:
Comment #26
omers CreditAttribution: omers commentedFollowing the instructions on #25 work's fine :)
Comment #28
daffie CreditAttribution: daffie commentedComment #29
omers CreditAttribution: omers commentedI 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 ?
Comment #30
shravan sonkar CreditAttribution: shravan sonkar commentedI am working on it
Comment #31
hussainwebI 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.
Comment #32
tstoecklerI 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, becausephpunit
already requires an adequate version ofphpunit-mock-objects.
Comment #33
hussainweb@tstoeckler: I changed it to
~2.4.2
which is equivalent to2.4.*
from earlier and still enforces the minimum version as2,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.
Comment #35
hussainwebIt looks like this is a random error. It tried to copy a file for migration and couldn't find it.
Comment #37
Mile23Are 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.Comment #38
hussainweb@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.
Comment #39
hussainweb@Mile23: As to your second comment:
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.Comment #40
tstoecklerHere we go. Here is proof that it is in fact OK to remove the explicit
phpunit/phpunit-mock-objects
declaration: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
.Comment #41
hussainweb@tstoeckler: I am not sure if this counts as proof. This might work and even the tests pass as
phpunit
depends onphpunit-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.
Comment #42
tstoecklerAhh 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.
Comment #43
Mile23Re:
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.Comment #44
tstoeckler@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.
Comment #45
Mile23OK. :-)
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-specificcomposer update
, I got a screen full of updates to just about every package:Comment #46
tstoeckler@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.
Comment #47
Mile23Really? Hmm.
Well, in that case, I'll RTBC and let the maintainer wrestle with this. :-)
Comment #48
hussainwebAs far as Symfony upgrades are concerned, there is an issue for that (TM): #2414235: Upgrade to Symfony 2.6.4
Comment #49
alexpottCommitted 61cb3e3 and pushed to 8.0.x. Thanks!
Thanks for adding the beta evaluation to the issue summary.