Skip to content

added an autoload-dev section #1344

Merged
merged 1 commit into from Mar 1, 2014
@bamarni
bamarni commented Nov 19, 2012

cf #1286. This should allow to configure a specific autoload section for development purpose.

Tests will heavily fail on Travis because I've also enabled this in composer's composer.json.

Edit: hum I think I should only commit the feature in itself for now, I'll revert back the composer test autoload while applying your other review comments if you have any

@Seldaek Seldaek commented on an outdated diff Nov 19, 2012
tests/bootstrap.php
@@ -13,4 +13,3 @@
error_reporting(E_ALL);
$loader = require __DIR__.'/../src/bootstrap.php';
-$loader->add('Composer\Test', __DIR__);
@Seldaek
Composer member
Seldaek added a note Nov 19, 2012

Can you leave this in for now so that travis passes (well.. it'll still fail but not ALL tests)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Seldaek
Composer member
Seldaek commented Nov 19, 2012

Still needs to be added in https://github.com/composer/composer/blob/master/src/Composer/Package/Dumper/ArrayDumper.php#L33 - apart from that and my small comment it looks good.

@bamarni
bamarni commented Nov 19, 2012

@Seldaek : here you go

@Baachi
Baachi commented Nov 27, 2012

:+1: to have this.

@bamarni
bamarni commented Nov 27, 2012

Btw I've just noticed there were some sorts of checks when dumping the classmap, avoiding test classes to be added (https://github.com/composer/composer/blob/master/src/Composer/Autoload/AutoloadGenerator.php#L125).

I don't know how efficient it is but in that case I'm wondering what is the point in recommending people to have a separate path for their tests? Maybe I'm missing something else but in the doc that is the reason I've mentionned.

@Seldaek
Composer member
Seldaek commented Nov 27, 2012

@bamarni that's because some packages include the tests in the same namespace/path as they have the sources, and scanning the tests makes it slower and adds garbage in the classmap, while it doesn't provide any benefit. But yes I guess that makes this PR sort of useless, and given that angle I have to say I'd rather not merge it to avoid adding complexity for nothing.

@bamarni
bamarni commented Nov 27, 2012

@Seldaek : I don't think saying it slows down the classmap generation is a valid argument, imo it only depends of the efficience of this filter, by quickly testing on the composer repo I can see there are only ~5 test files being dumped, while I must agree with you that it remains a small optimisation to setup a different path and a custom autoloader for testing purpose, it doesn't look worth the effort to me, but both solutions are ok.

And garbage in the classmap can't be reduced to test files, all the classes you never use, or (in a symfony2 project for example) all the classes from the di component (as it's not relevant at runtime), could also be considered as garbage, those kind of classes look way much like garbage to me than a few test classes, but unfortunately I don't think this can easily be addressed.

Anyway I agree with you that this PR can be closed.

@bamarni bamarni closed this Nov 27, 2012
@arjanvdbos arjanvdbos referenced this pull request in zendframework/zf2 Jul 2, 2013
@bakura10 bakura10 Remove ZendTest from Composer fc69a35
@holtkamp

I just encountered this PR while needing an 'autoload-dev' section. It seems it was declined, but still I would say the suggested 'autoload-dev' has added value. For two reasons:

Functionality used for test-bootstrapping / during testing
For instance, 'require-dev' can be used to include some testing frameworks like Behat for BDD. Autoloading of involved bootstrap classes would be usefull to only apply during development, and not when deployed on (cloud-based) applications servers:

"require-dev": {
    "behat/behat": "2.4.*@stable",
    "behat/mink": "1.4@stable",
    "behat/mink-goutte-driver": "*",
    "behat/mink-selenium2-driver": "*"
},
"autoload-dev": {
    "classmap": [
        "tests/features/bootstrap"
    ]
}

The lack of the autoload-dev section forces me to use the "autoload" section, which (on non-development environments) results in a rather harsh:

[RuntimeException]                                                                                            
remote:   Could not scan for classes inside "tests/features/bootstrap" which does not appear to be a file nor a folder  

Of course this can be done by adding such paths to the loader during test-bootstrapping, like Composer does, but this classloading "is" the whole idea of the composer autoload section, right?

Assumptions about naming conventions
IMHO the logic that strips all files with "Test" in its filename from the autoloading makes (too much?) assumptions about file naming conventions. An application which deals with test-suites probably has a lot of such files / classes... Not a big issue though, but its still an assumption.

@derrabus

:+1: I'd love to have that feature.

@YevKov
YevKov commented Jan 31, 2014

:+1: That what I need indeed!

@dchancogne

One more vote from me as well.

@VasekPurchart

:+1: , totally agree with reasons @holtkamp has given.

Composer should not by default filter out any class names. If there should be such a functionality at all, it should probably be configurable.

@aequasi
aequasi commented Feb 25, 2014

@Seldaek can we reopen this? The ability to only autoload tests in the dev environment would be nice

@Seldaek
Composer member
Seldaek commented Feb 25, 2014

Alright, let's reopen it, but it'll need to be rebased and possibly reworked a bit.

@Seldaek Seldaek reopened this Feb 25, 2014
@Seldaek
Composer member
Seldaek commented Feb 25, 2014

@bamarni would you still be interested in taking this on or should we call on someone else to finish it up?

@cordoval

:+1: if it does not add much complexity, if you are a dev you should work around or knowing your bootstrap scripts anyway, i am divided :baby:

@Seldaek Seldaek added this to the Nice To Have milestone Feb 25, 2014
@Seldaek Seldaek added the Feature label Feb 25, 2014
@Seldaek
Composer member
Seldaek commented Feb 25, 2014

@cordoval if you need classmap autoloading for the tests dir you can't do that in the bootstrap file.

@cordoval

you can't use ->addPSR4() ?? on the bootstrap file?

@Seldaek
Composer member
Seldaek commented Feb 25, 2014

You can, but if your files don't follow PSR-4 and you require classmap then you can't do addClassmap() and have it resolved at runtime, so that use case required autoload-dev.

@cordoval

oh i see, thanks for the explanation Jordi, let's move forward then

@stof
stof commented Feb 25, 2014

@Seldaek well, you can still use addClassMap on the class loader. The difference is that you have to generate the classmap yourself to pass it.

@Seldaek
Composer member
Seldaek commented Feb 25, 2014

Yes as I said you can't have it resolved at runtime so it's useless and this autoload-dev feature has some value.

@bamarni
bamarni commented Feb 26, 2014

@Seldaek : sure I'll rebase and see if it's still working. I'll take care of this tomorrow.

@bamarni
bamarni commented Feb 27, 2014

Here we go :)

@cordoval cordoval and 1 other commented on an outdated diff Feb 27, 2014
src/Composer/Package/PackageInterface.php
*
* @return array Mapping of autoloading rules
*/
public function getAutoload();
/**
+ * Returns an associative array of dev autoloading rules
+ *
+ * {"<type>": {"<namespace": "<directory>"}}
+ *
+ * Type is either "psr-0", "classmap" or "files". Namespaces are mapped to
@cordoval
cordoval added a note Feb 27, 2014

shouldn't psr-4 also be included here? sorry if it is a dumb question

@bamarni
bamarni added a note Feb 27, 2014

good catch! back then when I first made this PR it didn't existed yet... fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@cordoval cordoval commented on the diff Feb 27, 2014
tests/bootstrap.php
@@ -13,6 +13,8 @@
error_reporting(E_ALL);
$loader = require __DIR__.'/../src/bootstrap.php';
+
+// to be removed
@cordoval
cordoval added a note Feb 27, 2014

:+1:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@Seldaek
Composer member
Seldaek commented Mar 1, 2014

Merged thanks

@Seldaek Seldaek merged commit db91454 into composer:master Mar 1, 2014

1 check passed

Details default The Travis CI build passed
@aequasi
aequasi commented Mar 1, 2014

:thumbsup: thanks @Seldaek for reopening, and @bamarni for the extra work :)

@aequasi aequasi referenced this pull request in keenlabs/KeenClient-PHP Mar 5, 2014
Merged

Moving code to PSR-4 #50

@staabm

This sounds like it will only work with psr-0 name schema,...

@Maks3w Maks3w referenced this pull request in zendframework/zf2 Apr 30, 2014
Closed

Use composer's autoload-dev feature #6205

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.