Skip to content
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

Support strict (no ambiguous classes) mode for generating optimised autoloader #6221

Open
acoulton opened this issue Mar 2, 2017 · 14 comments
Labels
Milestone

Comments

@acoulton
Copy link
Contributor

acoulton commented Mar 2, 2017

I'd love a composer install --optimize-autoloader --prevent-ambiguous (or similar named) option to make the install fail (rather than just print a warning) if it finds ambiguous class definitions.

The resolution of ambiguous class names is platform-dependent (the order composer finds the files varies between filesystems) which can lead to composer autoloading a totally different implementation of a class between development / build / production instances. Similarly classes contained in files that don't match the PSR-0/4 definition won't cause issues locally but may be found and used when the classmap is generated. For example this can happen if working / backup files are accidentally committed, or if a (badly-configured) dependency with a loose psr-0 definition comes with test / example files.

It can be very hard to debug and trace the problems this causes. They can arise at any time (even if the composer dependencies don't actually change), so a developer might not always notice them. Often the "Ambiguous class resolution" warnings will only arise within a remote build / deployment process and not be noticed - particularly if the composer command output doesn't appear in deployment logs.

So I'd like to be able to opt-in to have composer fail with an error if ambiguous classes are found, to force the end-user to fix that rather than hope the resolution is always going to be correct now and in future.

I'm happy to work on a PR if you agree this is a good idea?

@alcohol alcohol added the Feature label Mar 2, 2017
@alcohol
Copy link
Member

alcohol commented Mar 2, 2017

I am not opposed to this, I am just not sure if it should be --prevent-ambiguous or if maybe a more generic --strict would be more suited.

@acoulton
Copy link
Contributor Author

acoulton commented Mar 2, 2017

Yeah I wondered about --strict too, my only reservation was whether then that would create an obligation over time to make sure it was strict about everything to avoid confusion about what it was / wasn't enforcing.
Possibly the simplest way to do a generic --strict would be to bind to the IOInterface somehow and fail if any warnings or errors were displayed - although from a quick scan of the code it looks like io->writeError is used for quite a bit of non-error informational and debug logging too so that might be more complex than it seems.

@alcohol
Copy link
Member

alcohol commented Mar 2, 2017

That's because stderr is not only for errors, unlike a lot of people seem to think. Writing to stderr does not imply something went wrong and it is a horrible way to make bad assumptions.

@acoulton
Copy link
Contributor Author

acoulton commented Mar 2, 2017

That's true, though arguably fair enough given the misleading "error stream" naming. But as an outsider when I looked at a php method called ->writeError I wasn't thinking about shell-level stderr. I'd thought / hoped it was more of a higher-level severity method. Anyway, I'm not here to argue about naming.

Very happy to look at suggestions for how I could implement a robust and future-proof generic --strict flag. Possibly still at the IO level but based on the <warning> etc formatting tags in the messages themselves rather than which stream they're on?

@alcohol
Copy link
Member

alcohol commented Mar 2, 2017

Unfortunately I do not think it will be quite that simple. Cannot think of a proper solution of the top of my head either though.

@acoulton
Copy link
Contributor Author

acoulton commented Mar 2, 2017

Just to confirm that definitely won't work - I just noticed abandoned packages are displayed as warnings with equal severity/formatting to ambiguous classes. It looks to me like adding a generic --strict option would be an order of magnitude larger than what I'd envisaged just to enforce no ambiguous classes.

What other warnings does composer issue during install that might actually have an impact on (and particularly, platform-specific impact) production behaviour? I'm not really aware of anything else that it would be important for --strict to catch at the moment - so maybe a specific flag is more appropriate after all?

@Seldaek
Copy link
Member

Seldaek commented Mar 6, 2017

I also don't think it's worth having a --strict, given that the scope might change over time, it's best to have specific flags.

Perhaps --classmap-strict would be good/short enough?

@Seldaek Seldaek added this to the Nice To Have milestone Mar 6, 2017
@acoulton
Copy link
Contributor Author

acoulton commented Mar 6, 2017

--classmap-strict seems good to me.

@ashnazg
Copy link

ashnazg commented Aug 1, 2018

Just stumbled into this need... is this feature on hold?

@alcohol
Copy link
Member

alcohol commented Aug 2, 2018

Development of nice-to-have features is very much up to opensource contributions. Feel free to submit a PR.

@Seldaek
Copy link
Member

Seldaek commented Aug 3, 2018

It's somewhat related to #7421

@ashnazg
Copy link

ashnazg commented Aug 3, 2018

I toyed with adding this config option yesterday, but couldn't see a direct way to access the setting at the two points where the Ambiguous warning is recognized, much less how to set the execution as "failed" from the same points.

@fredden
Copy link
Contributor

fredden commented Jul 19, 2023

--strict-psr was introduced in #10886, which landed in Composer version 2.4.0. Does that satisfy this feature request?

@Seldaek
Copy link
Member

Seldaek commented Jul 19, 2023

I don't think so, this is about ambiguous classes (class names present more than once), not PSR validity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants