New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PSR-4: ClassLoader + AutoloadGenerator + tests #2459

Merged
merged 16 commits into from Jan 3, 2014

Conversation

Projects
None yet
9 participants
@donquixote
Contributor

donquixote commented Nov 26, 2013

Here we go!

This time without any refactoring in AutoloadGenerator or AutoloadGeneratorTests. This is still a mess!

Based on a rebase of #2295, which was a follow-up to #2121

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Nov 26, 2013

Contributor

How can we force Travis CI to do bin/composer install and not just composer install?

Contributor

donquixote commented Nov 26, 2013

How can we force Travis CI to do bin/composer install and not just composer install?

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Nov 26, 2013

Contributor

ah, I get it! Edit .travis.yml.

Contributor

donquixote commented Nov 26, 2013

ah, I get it! Edit .travis.yml.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Nov 26, 2013

Contributor

@donquixote you need to install the vendors first before being able to use composer from source

Contributor

stof commented Nov 26, 2013

@donquixote you need to install the vendors first before being able to use composer from source

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Nov 26, 2013

Contributor

@stof Show me how!

Contributor

donquixote commented Nov 26, 2013

@stof Show me how!

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Nov 26, 2013

Contributor

@donquixote install deps with the working composer (i.e the system-wide one) and then dump the autoload again with the local one (no need to do the full installation again. the autoload generation is enough)

Contributor

stof commented Nov 26, 2013

@donquixote install deps with the working composer (i.e the system-wide one) and then dump the autoload again with the local one (no need to do the full installation again. the autoload generation is enough)

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Nov 26, 2013

Contributor

Ok, so what to write in .travis.yml?

Contributor

donquixote commented Nov 26, 2013

Ok, so what to write in .travis.yml?

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Nov 27, 2013

Contributor

I don't get those fails.
It seems that at some point, autoload_real.php is the one with PSR-4, but autoload_psr4.php is missing. But I don't understand why that would happen. If you run bin/composer install locally, the autoload_psr4.php is created, even if it returns an empty array.

Contributor

donquixote commented Nov 27, 2013

I don't get those fails.
It seems that at some point, autoload_real.php is the one with PSR-4, but autoload_psr4.php is missing. But I don't understand why that would happen. If you run bin/composer install locally, the autoload_psr4.php is created, even if it returns an empty array.

@Seldaek

This comment has been minimized.

Show comment
Hide comment
@Seldaek

Seldaek Nov 27, 2013

Member

@donquixote the AllFunctional tests use a phar compiled from the current checkout so this should indeed be correct if you only run bin/composer dump-autoload before. Strange but in the worst case I can live with travis failing those tests until the getcomposer.org phar is updated.

Member

Seldaek commented Nov 27, 2013

@donquixote the AllFunctional tests use a phar compiled from the current checkout so this should indeed be correct if you only run bin/composer dump-autoload before. Strange but in the worst case I can live with travis failing those tests until the getcomposer.org phar is updated.

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Nov 28, 2013

Contributor

@Seldaek Could you have a special look on the testMainPackageAutoloadingWithTargetDir() test, if this is the intended behavior with PSR-4 plus target-dir setting?

Btw, I am still a little confused by the fixtures all thrown in one folder.. This is also asking for refactoring (after PSR-4 is in). Maybe one folder per "scenario", and then name the files by their actual expected name.

Contributor

donquixote commented Nov 28, 2013

@Seldaek Could you have a special look on the testMainPackageAutoloadingWithTargetDir() test, if this is the intended behavior with PSR-4 plus target-dir setting?

Btw, I am still a little confused by the fixtures all thrown in one folder.. This is also asking for refactoring (after PSR-4 is in). Maybe one folder per "scenario", and then name the files by their actual expected name.

@Seldaek

This comment has been minimized.

Show comment
Hide comment
@Seldaek

Seldaek Nov 29, 2013

Member

@donquixote IMO psr-4 should just be disallowed with target-dir. We should only allow target-dir with psr-0 ideally, but I don't know if that'd break existing packages so it's safer not to. But I would disallow it with psr-4, and deprecate it once psr-4 is merged because psr-4 is preferrable to psr-0 + target-dir really.

Member

Seldaek commented Nov 29, 2013

@donquixote IMO psr-4 should just be disallowed with target-dir. We should only allow target-dir with psr-0 ideally, but I don't know if that'd break existing packages so it's safer not to. But I would disallow it with psr-4, and deprecate it once psr-4 is merged because psr-4 is preferrable to psr-0 + target-dir really.

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Nov 30, 2013

Contributor

I imagine that some projects might want to switch to PSR-4 even without
moving their files around, if only for a small performance benefit.
E.g.
'psr-4': 'Symfony\Foo' => 'lib/Symfony/Foo/',

I don't know if this would make any sense for projects with target-dir..

On 29 November 2013 20:48, Jordi Boggiano notifications@github.com wrote:

@donquixote https://github.com/donquixote IMO psr-4 should just be
disallowed with target-dir. We should only allow target-dir with psr-0
ideally, but I don't know if that'd break existing packages so it's safer
not to. But I would disallow it with psr-4, and deprecate it once psr-4 is
merged because psr-4 is preferrable to psr-0 + target-dir really.


Reply to this email directly or view it on GitHubhttps://github.com/composer/composer/pull/2459#issuecomment-29534900
.

Contributor

donquixote commented Nov 30, 2013

I imagine that some projects might want to switch to PSR-4 even without
moving their files around, if only for a small performance benefit.
E.g.
'psr-4': 'Symfony\Foo' => 'lib/Symfony/Foo/',

I don't know if this would make any sense for projects with target-dir..

On 29 November 2013 20:48, Jordi Boggiano notifications@github.com wrote:

@donquixote https://github.com/donquixote IMO psr-4 should just be
disallowed with target-dir. We should only allow target-dir with psr-0
ideally, but I don't know if that'd break existing packages so it's safer
not to. But I would disallow it with psr-4, and deprecate it once psr-4 is
merged because psr-4 is preferrable to psr-0 + target-dir really.


Reply to this email directly or view it on GitHubhttps://github.com/composer/composer/pull/2459#issuecomment-29534900
.

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Nov 30, 2013

Contributor

Looking at https://github.com/symfony/WebProfilerBundle/blob/1a9a3613a5ca4ca09a5a62d8c854e9e7ae628ef0/composer.json

Code right now:

"autoload": {
    "psr-0": { "Symfony\\Bundle\\WebProfilerBundle\\": "" }
},
"target-dir": "Symfony/Bundle/WebProfilerBundle",

This could easily be replaced by

"autoload": {
    "psr-4": { "Symfony\\Bundle\\WebProfilerBundle\\": "" }
}

without any target-dir setting, and without moving any files around.
So yes, I am all for it!

Maybe we should throw an exception or make some other kind of noise if we find target-dir with PSR-4.

Contributor

donquixote commented Nov 30, 2013

Looking at https://github.com/symfony/WebProfilerBundle/blob/1a9a3613a5ca4ca09a5a62d8c854e9e7ae628ef0/composer.json

Code right now:

"autoload": {
    "psr-0": { "Symfony\\Bundle\\WebProfilerBundle\\": "" }
},
"target-dir": "Symfony/Bundle/WebProfilerBundle",

This could easily be replaced by

"autoload": {
    "psr-4": { "Symfony\\Bundle\\WebProfilerBundle\\": "" }
}

without any target-dir setting, and without moving any files around.
So yes, I am all for it!

Maybe we should throw an exception or make some other kind of noise if we find target-dir with PSR-4.

@webchick

This comment has been minimized.

Show comment
Hide comment
@webchick

webchick Dec 6, 2013

Looks like PSR-4 passed: https://groups.google.com/forum/#!msg/php-fig/L8oCDQCzDcs/Z-6_9U0hpJIJ Hooray!

Is there a timeframe on when this is planning to get merged into Composer's autoloader?

webchick commented Dec 6, 2013

Looks like PSR-4 passed: https://groups.google.com/forum/#!msg/php-fig/L8oCDQCzDcs/Z-6_9U0hpJIJ Hooray!

Is there a timeframe on when this is planning to get merged into Composer's autoloader?

@dongilbert

This comment has been minimized.

Show comment
Hide comment
@dongilbert

dongilbert Dec 6, 2013

"Maybe we should throw an exception or make some other kind of noise if we find target-dir with PSR-4."

I would say no. It's a human that's involved, and made decision to do both PSR-4 AND put a target-dir, so I don't think we should do anything about it, except maybe document the practice.

dongilbert commented Dec 6, 2013

"Maybe we should throw an exception or make some other kind of noise if we find target-dir with PSR-4."

I would say no. It's a human that's involved, and made decision to do both PSR-4 AND put a target-dir, so I don't think we should do anything about it, except maybe document the practice.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 6, 2013

Contributor

@dongilbert a human can do mistakes, especially when it is abotu taking a decision related to an external tool. We already saw lots of misunderstanding of target-dir so warning about mistakes is a good idea IMO (the behavior when using both together will be quite hard to understand IMO, for a case which does not make much sense).
Documenting a practice is not enough to avoid people using it, because many people don't read the doc properly. The real benefit is that maintainers can close issues with a link to the doc instead of having to write an explanation again and again.

I can guess that even if we throw an exception with a message explaining why it is there, we would still see some issues opened for it. Unfortunately, some people don't even read the error message they are getting when they copy-paste it in an issue tracker.

Contributor

stof commented Dec 6, 2013

@dongilbert a human can do mistakes, especially when it is abotu taking a decision related to an external tool. We already saw lots of misunderstanding of target-dir so warning about mistakes is a good idea IMO (the behavior when using both together will be quite hard to understand IMO, for a case which does not make much sense).
Documenting a practice is not enough to avoid people using it, because many people don't read the doc properly. The real benefit is that maintainers can close issues with a link to the doc instead of having to write an explanation again and again.

I can guess that even if we throw an exception with a message explaining why it is there, we would still see some issues opened for it. Unfortunately, some people don't even read the error message they are getting when they copy-paste it in an issue tracker.

@dongilbert

This comment has been minimized.

Show comment
Hide comment
@dongilbert

dongilbert Dec 9, 2013

As long as it's a warning, and not something that stops the whole process. I tend to think, however, that the users will figure out they did something wrong when their code doesn't autoload. They can then look at the docs for using PSR-4 with Composer, and see their mistake if it was indeed a mistake.

Maybe it would be beneficial to output links to documentation items in the displayed Exception, to make them one less google search away from the answer they require.

dongilbert commented Dec 9, 2013

As long as it's a warning, and not something that stops the whole process. I tend to think, however, that the users will figure out they did something wrong when their code doesn't autoload. They can then look at the docs for using PSR-4 with Composer, and see their mistake if it was indeed a mistake.

Maybe it would be beneficial to output links to documentation items in the displayed Exception, to make them one less google search away from the answer they require.

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Dec 9, 2013

Contributor

i am going to produce something tonight.
i think an exception is the way to go, with a message as helpful as
possible. Like "Package Symfony/EvilBundle has a composer.json that uses
both PSR-4 and target-dir setting. This combination is not supported by
Composer."

The goal is not to punish the user, but to step on the feet of the package
maintainer, with the goal that a package with this combination will never
be released.

2013/12/9 Don Gilbert notifications@github.com

As long as it's a warning, and not something that stops the whole process.
I tend to think, however, that the users will figure out they did something
wrong when their code doesn't autoload. They can then look at the docs for
using PSR-4 with Composer, and see their mistake.

Maybe it would be beneficial to output links to documentation items in the
displayed Exception, to make them one less google search away from the
answer they require.


Reply to this email directly or view it on GitHubhttps://github.com/composer/composer/pull/2459#issuecomment-30135846
.

Contributor

donquixote commented Dec 9, 2013

i am going to produce something tonight.
i think an exception is the way to go, with a message as helpful as
possible. Like "Package Symfony/EvilBundle has a composer.json that uses
both PSR-4 and target-dir setting. This combination is not supported by
Composer."

The goal is not to punish the user, but to step on the feet of the package
maintainer, with the goal that a package with this combination will never
be released.

2013/12/9 Don Gilbert notifications@github.com

As long as it's a warning, and not something that stops the whole process.
I tend to think, however, that the users will figure out they did something
wrong when their code doesn't autoload. They can then look at the docs for
using PSR-4 with Composer, and see their mistake.

Maybe it would be beneficial to output links to documentation items in the
displayed Exception, to make them one less google search away from the
answer they require.


Reply to this email directly or view it on GitHubhttps://github.com/composer/composer/pull/2459#issuecomment-30135846
.

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Dec 12, 2013

Contributor

I can't figure out what's the best place to throw such an exception. Maybe even more than one place.
igorw and simensen mentioned Composer\Json\JsonFile::validateSchema().

Contributor

donquixote commented Dec 12, 2013

I can't figure out what's the best place to throw such an exception. Maybe even more than one place.
igorw and simensen mentioned Composer\Json\JsonFile::validateSchema().

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 13, 2013

Contributor

it is not in JsonFile::validateSchema (which is about applying the json schema validation).

the good place for this validation is in the ValidatingArrayLoader

Contributor

stof commented Dec 13, 2013

it is not in JsonFile::validateSchema (which is about applying the json schema validation).

the good place for this validation is in the ValidatingArrayLoader

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Dec 13, 2013

Contributor

Are you sure this validation happens every time someone runs composer
install?

On 13 December 2013 11:31, Christophe Coevoet notifications@github.comwrote:

it is not in JsonFile::validateSchema (which is about applying the json
schema validation).

the good place for this validation is in the ValidatingArrayLoaderhttps://github.com/composer/composer/blob/master/src/Composer/Package/Loader/ValidatingArrayLoader.php


Reply to this email directly or view it on GitHubhttps://github.com/composer/composer/pull/2459#issuecomment-30499852
.

Contributor

donquixote commented Dec 13, 2013

Are you sure this validation happens every time someone runs composer
install?

On 13 December 2013 11:31, Christophe Coevoet notifications@github.comwrote:

it is not in JsonFile::validateSchema (which is about applying the json
schema validation).

the good place for this validation is in the ValidatingArrayLoaderhttps://github.com/composer/composer/blob/master/src/Composer/Package/Loader/ValidatingArrayLoader.php


Reply to this email directly or view it on GitHubhttps://github.com/composer/composer/pull/2459#issuecomment-30499852
.

@stof

This comment has been minimized.

Show comment
Hide comment
@stof

stof Dec 13, 2013

Contributor

@donquixote It does not for packages coming from repositories. But for them, it is run by packagist when updating them. So in this case, it will notify maintainers about their mistake instead of notifying users (which cannot really fix it).
and it is also by composer validate

Contributor

stof commented Dec 13, 2013

@donquixote It does not for packages coming from repositories. But for them, it is run by packagist when updating them. So in this case, it will notify maintainers about their mistake instead of notifying users (which cannot really fix it).
and it is also by composer validate

@philsturgeon

This comment has been minimized.

Show comment
Hide comment
@philsturgeon

philsturgeon Dec 14, 2013

Where are we at with this chaps?

philsturgeon commented Dec 14, 2013

Where are we at with this chaps?

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Dec 14, 2013

Contributor

@philsturgeon: maybe some people should try this at home, in addition to the travis/phpunit stuff.

Contributor

donquixote commented Dec 14, 2013

@philsturgeon: maybe some people should try this at home, in addition to the travis/phpunit stuff.

@machee

This comment has been minimized.

Show comment
Hide comment
@machee

machee Dec 18, 2013

Contributor

Small test but figured I'd give some feedback.

Cloned and compiled donquixote/composer@c0aad84 from feature/psr4-complete branch. Changed composer.json in the same manner as in this comment for a package in our satis repos. Then composer update and everything working as expected.

Contributor

machee commented Dec 18, 2013

Small test but figured I'd give some feedback.

Cloned and compiled donquixote/composer@c0aad84 from feature/psr4-complete branch. Changed composer.json in the same manner as in this comment for a package in our satis repos. Then composer update and everything working as expected.

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Dec 22, 2013

Contributor

Thanks machee!

Quick note about the 2x validation:
The one in ValidatingArrayLoader is to tell the package maintainer directly that something is wrong.

The validation in AutoloadGenerator is to tell the package user that the package is "broken". The package user can:

  • report the bug to the package maintainer, and
  • temporarily fork the package, and/or
  • temporarily stop using this package.

All of this is better than silently pretending that everything is ok.

Contributor

donquixote commented Dec 22, 2013

Thanks machee!

Quick note about the 2x validation:
The one in ValidatingArrayLoader is to tell the package maintainer directly that something is wrong.

The validation in AutoloadGenerator is to tell the package user that the package is "broken". The package user can:

  • report the bug to the package maintainer, and
  • temporarily fork the package, and/or
  • temporarily stop using this package.

All of this is better than silently pretending that everything is ok.

@donquixote

This comment has been minimized.

Show comment
Hide comment
@donquixote

donquixote Dec 22, 2013

Contributor

I would like to also add a PSR-4 case in AutoloadGeneratorTest::testOverrideVendorsAutoloading().
But I don't really get the idea of this test. I suppose it is meant to test the order and priority of namespace mappings. But I don't quite get the logic behind this order. My guess is:

/**
 * Test that PSR-0 and PSR-4 mappings are processed in the correct order for
 * autoloading and for classmap generation:
 * - The main package has priority over other packages.
 * - Longer namespaces have priority over shorter namespaces.
 */

But I don't see this documented anywhere.. could be I am wrong and it is just the order of packages that matters.

Contributor

donquixote commented Dec 22, 2013

I would like to also add a PSR-4 case in AutoloadGeneratorTest::testOverrideVendorsAutoloading().
But I don't really get the idea of this test. I suppose it is meant to test the order and priority of namespace mappings. But I don't quite get the logic behind this order. My guess is:

/**
 * Test that PSR-0 and PSR-4 mappings are processed in the correct order for
 * autoloading and for classmap generation:
 * - The main package has priority over other packages.
 * - Longer namespaces have priority over shorter namespaces.
 */

But I don't see this documented anywhere.. could be I am wrong and it is just the order of packages that matters.

@donquixote donquixote referenced this pull request Dec 26, 2013

Open

PSR4 #21

@ghost ghost referenced this pull request Dec 30, 2013

Merged

Add support for PSR-4 #1436

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost Dec 31, 2013

What's holding up this PR?

ghost commented Dec 31, 2013

What's holding up this PR?

@philsturgeon

This comment has been minimized.

Show comment
Hide comment
@philsturgeon

philsturgeon Dec 31, 2013

I suppose we need some more +1's from people who have tried it. I'll admit I should have, I'll give it a go now.

philsturgeon commented Dec 31, 2013

I suppose we need some more +1's from people who have tried it. I'll admit I should have, I'll give it a go now.

philsturgeon pushed a commit to thephpleague/fractal that referenced this pull request Dec 31, 2013

Phil Sturgeon
Switched from PSR-0 to PSR-4.
Also testing PSR-4 pull request for composer: composer/composer#2459
@philsturgeon

This comment has been minimized.

Show comment
Hide comment
@philsturgeon

philsturgeon Dec 31, 2013

I've implemented a PSR-4 branch in a package I've been working on and the unit-tests are passing nicely. Sorry I don't have anything else bigger and more tested ready to play with, but it looks good. See above.

Can we merge this?

philsturgeon commented Dec 31, 2013

I've implemented a PSR-4 branch in a package I've been working on and the unit-tests are passing nicely. Sorry I don't have anything else bigger and more tested ready to play with, but it looks good. See above.

Can we merge this?

@igorw

This comment has been minimized.

Show comment
Hide comment
@igorw

igorw Dec 31, 2013

Contributor

Patch looks good.

Contributor

igorw commented Dec 31, 2013

Patch looks good.

@Seldaek Seldaek merged commit 0a4b18c into composer:master Jan 3, 2014

1 check passed

default The Travis CI build passed
Details
@Seldaek

This comment has been minimized.

Show comment
Hide comment
@Seldaek

Seldaek Jan 3, 2014

Member

Merged with just a few tweaks (see above) and I also updated the docs to recommend psr-4 and other shenanigans. Thanks @donquixote! Please mind the blog post at http://seld.be/notes/psr-4-autoloading-support-in-composer and don't rush on this to avoid creating a support mess + pissed off users :)

Member

Seldaek commented Jan 3, 2014

Merged with just a few tweaks (see above) and I also updated the docs to recommend psr-4 and other shenanigans. Thanks @donquixote! Please mind the blog post at http://seld.be/notes/psr-4-autoloading-support-in-composer and don't rush on this to avoid creating a support mess + pissed off users :)

@ceeram

This comment has been minimized.

Show comment
Hide comment
@ceeram

ceeram Apr 4, 2014

Nitpick, opening brace should be on a new line

Nitpick, opening brace should be on a new line

Michelemassari pushed a commit to Michelemassari/fractal that referenced this pull request Apr 8, 2014

Phil Sturgeon
Switched from PSR-0 to PSR-4.
Also testing PSR-4 pull request for composer: composer/composer#2459

aminztw pushed a commit to aminztw/composer that referenced this pull request Jan 1, 2017

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