Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Composer modify #344

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
5 participants

Hounddog commented May 2, 2012

Included Bin file into composer
Modified bin dependencies to inclue the Composer Autoload.php
Added Missing Symfony/Console Dependency

Collaborator

Ocramius commented on bin/doctrine.php in a630272 May 2, 2012

Skip these by putting 'em into an else :)

Owner

Ocramius commented May 2, 2012

Absolutely +1 :)
Now another nasty request. Do you think it could work also with https://github.com/doctrine/doctrine2/blob/32b146ea8ad9484bfbb522b3d7ead8f01803a628/tests/Doctrine/Tests/TestInit.php ?

@Burgov Burgov and 1 other commented on an outdated diff May 2, 2012

composer.json
@@ -14,9 +14,11 @@
"require": {
"php": ">=5.3.2",
"ext-pdo": "*",
- "doctrine/dbal": "dev-master"
+ "doctrine/dbal": "dev-master",
+ "symfony/console" : "2.0.12"
@Burgov

Burgov May 2, 2012

Contributor

indentation is broken here

@Hounddog

Hounddog May 2, 2012

didn't see that... just edited with vim and had put a tab there. fixed now

@stof stof and 1 other commented on an outdated diff May 2, 2012

bin/doctrine.php
+ $classLoader = new \Doctrine\Common\ClassLoader('Symfony', 'Doctrine');
@stof

stof May 2, 2012

Member

this should probably be changed as the console component is not in the Doctrine folder (it was the case for the first Doctrine pear package when symfony2 was not available through pear yet and so doctrine maintained their own one)

@Hounddog

Hounddog May 2, 2012

Which would suggest that we can just remove Symfony. If i read this correctly

@stof

stof May 3, 2012

Member

no. You need to register an autoloader for the Symfony namespace, but you should not target a Doctrine folder for this as it will not be installed in Doctrine/Symfony/Component/ when installing through PEAR now

@stof stof and 1 other commented on an outdated diff May 2, 2012

composer.json
@@ -14,9 +14,11 @@
"require": {
"php": ">=5.3.2",
"ext-pdo": "*",
- "doctrine/dbal": "dev-master"
+ "doctrine/dbal": "dev-master",
+ "symfony/console" : "2.0.12"
@stof

stof May 2, 2012

Member

symfony/console should be in the suggest section (as it is optional, even if using the CLI tool is recommended) and eventually in require-dev (if you want it for the tests but the testsuite does not rely on composer currently). And the constraint is too strict. You should not force using only the 2.0.12 tag of Symfony as it would force changing Doctrine each time a bugfix release of Symfony is done (for instance now as the latest is 2.0.13) and would break things if you have a library forcing to use 2.0.12 and the other one forcing to use 2.0.13 only (impossible to use them together whereas they would generally work with any 2.0.x release as they are BC)

@Hounddog

Hounddog May 2, 2012

I agree, was a mistake of mine. I would put it to 2.0.x then if this is prefered :)
Am also modifying the tests to work with composer. By adding the autoload.php to them.

@stof

stof May 3, 2012

Member

2.0.* is not enough either. Doctrine is fully compatible with the upcoming 2.1 component (as there is no BC break in the Console component) and requiring the 2.0 component would forbid using Doctrine master with Symfony 2.1. And anyway, it should not be a hard requirement here. The explanation about the strict constraint was mainly about educating.

Owner

Ocramius commented May 4, 2012

@stof: just been browsing through the tests and fixing @Hounddog's pr a bit. I think Doctrine should probably start to define symfony/console as a real dependency. Tests also depend on it. I see no use in Doctrine without its CLI except for deployed installations, but even there you tend to need it to check the environment (orm:ensure-production-settings).

Thoughts?

Member

stof commented May 4, 2012

@Ocramius If tests need them, it should go in require-dev (except that the testsuite does not use Composer but submodules so it is off topic anyway). And the Console component is indeed highly recommended, but it is still an optional dependency as someone could choose not to use the console (for instance if he does not have a cli access)

Owner

Ocramius commented May 4, 2012

@stof makes sense. Anyway, it was more to be able to run the test suite for cases where oh noes! doctrine is failing me and you just want to check things real quick :) Actually, anyone should do that before reporting a bug, no?

Member

stof commented May 4, 2012

@Ocramius If things are needed for the testsuite but are not required deps, they should go in the require-dev section to be installed when running composer with the --dev option. And as I said before, this will not help for Doctrine as the testsuite assumes that the vendors are in the location provided by the submodules, not in the location provided by composer

Owner

Ocramius commented May 4, 2012

@stof test suite works more than fine with just the composer installation (no lib/vendor assumptions) :)
I think it is a good addition to immediately have an overview about a breaking/too lazy check on a dependency version :)

Member

stof commented May 4, 2012

@Ocramius there is an assumption: see the file used as phpunit bootstrap

Owner

Ocramius commented May 4, 2012

@stof in fact, this has been handled in this PR at 3fbb132

@stof stof commented on the diff May 4, 2012

bin/doctrine.php
@@ -17,14 +17,18 @@
* <http://www.doctrine-project.org>.
*/
-require_once 'Doctrine/Common/ClassLoader.php';
+if (file_exists(__DIR__ . '/../../../autoload.php')) {
+ // doctrine is part of a composer installation
+ require_once __DIR__ . '/../../../autoload.php';
@stof

stof May 4, 2012

Member

this will fail if you get Doctrine and run composer to get its deps as it will be in ../vendor/autoload.php

@Ocramius

Ocramius May 4, 2012

Owner

Thought about that case, but I decided to not consider it. I can add it again though :)

@stof

stof May 4, 2012

Member

well, it is more logical to support this case than the case of Doctrine being installed as a deps of your project IMO, as --dev only installs deps for the root package, meaning the testsuite will work only if your project requires Console in some way

@stof

stof May 4, 2012

Member

hmm sorry, in bin/doctrine.php, it is not really needed. but the TestInit.php file should handle it

@Ocramius

Ocramius May 4, 2012

Owner

'alright :)
Will do later!

Owner

beberlei commented May 22, 2012

We don't test with composer, dependencies for dev mode are grabbed through submodules. The changes in the doctrine.php are not what i would do. I will close and fix this myself. Sorry for being so rude about it :-)

@beberlei beberlei closed this May 22, 2012

Owner

Ocramius commented May 23, 2012

@beberlei no problem :)

Owner

Ocramius commented May 23, 2012

@beberlei would you reconsider if it is rebased without the changes under tests/ ?

Owner

beberlei commented May 23, 2012

I already implemented this in a commit yesterday :-)

Owner

Ocramius commented May 23, 2012

@beberlei nice :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment