Skip to content

avoid duplicated path with addPrefix #15161

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

Closed
wants to merge 1 commit into from

Conversation

remicollet
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets [n/a
License MIT
Doc PR n/a

Having UniversalClassLoader deprecated raise various issues... and ClassLoader doesn't provide the same features... :(

This one avoid duplication of paths for a given prefix, so can improve perf is such case.

Notice: I open this PR against 2.7, first version where UniversalClassLoader is deprecated, but feel free to apply only if 2.8 or 3.0, as you prefer.

Context on how this class is used in various fedora packages, see
http://blog.remirepo.net/post/2015/06/30/PHP-SIG-autoloader

@stof
Copy link
Member

stof commented Jul 1, 2015

Which features are not provided by the ClassLoader ?

@stof
Copy link
Member

stof commented Jul 1, 2015

regarding the explanations in your blog post, the difference between namespace prefix and class prefix for non-namespaced classes was an unnecessary complexity in the UniversalClassLoader. In both cases, it is just a prefix of the class name. this is why Composer dropped this distinction and then we dropped it also in our new implementation.
and for PSR-4 support, this is implemented by the Psr4ClassLoader in Symfony. The ClassLoader is only about PSR-0 (Composer decided to use a single class for both, with different setters instead)

@remicollet
Copy link
Contributor Author

Which features are not provided by the ClassLoader ?

With UniversalClassLoader, last call to registerNamespace() have prioritty, while with ClassLoader first call to addPrefix() have priority. This is a really different behavior (Adding vs Setting)

With UniversalClassLoader we have different registration for namsepace and prefix.
So an additional trailing \ is needed when using ClassLoader::addPrefix en ensure correct behavior.

And with UniversalClassLoader no duplicated path is possible, while with ClassLoader you can add various time the same prefix with the path (the reason of this PR)

P.S. I don't ask to drop deprecation of UniversalClassLoader, we can live with ClassLoader (once the differences are understood), I just like to fix this duplicate path / perf issue.

@stof
Copy link
Member

stof commented Jul 1, 2015

@remicollet technically, UniversalClassLoader still allows to have several times the same path for a prefix. It just does not care about it. The difference is that it overwrites the existing paths each time, so each call has to pass all paths for a given prefix rather than being able to add new ones without knowing the current state

@remicollet
Copy link
Contributor Author

@stof, so we agree UniversalClassLoader and ClassLoader are different. And this is not the goal of this PR to change this.

Any feedback on this PR ?

@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

Thank you @remicollet.

fabpot added a commit that referenced this pull request Oct 6, 2015
This PR was submitted for the 2.7 branch but it was merged into the 2.3 branch instead (closes #15161).

Discussion
----------

avoid duplicated path with addPrefix

 Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | [n/a
| License       | MIT
| Doc PR        | n/a

Having UniversalClassLoader deprecated raise various issues... and ClassLoader doesn't provide the same features... :(

This one avoid duplication of paths for a given prefix, so can improve perf is such case.

Notice: I open this PR against 2.7, first version where UniversalClassLoader is deprecated, but feel free to apply only if 2.8 or 3.0, as you prefer.

Context on how this class is used in various fedora packages, see
http://blog.remirepo.net/post/2015/06/30/PHP-SIG-autoloader

Commits
-------

af420c1 avoid duplicated path with addPrefix
@fabpot
Copy link
Member

fabpot commented Oct 6, 2015

merged into 2.3, which is still maintained.

@fabpot fabpot closed this Oct 6, 2015
@remicollet
Copy link
Contributor Author

Thanks a lot.

This was referenced Oct 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants