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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DebugClassLoader] expose proxyfied findFile() method #29833

Merged
merged 1 commit into from Jan 13, 2019

Conversation

Projects
None yet
5 participants
@fancyweb
Copy link
Contributor

fancyweb commented Jan 10, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

As bad as it is, some third party libraries expect that at least one autoload function will be the Composer one and have behaviors that relies on the public findFile method.

When the DebugClassLoader wraps the Composer ClassLoader, the function findFile is currently lost. So it becomes impossible to use the DebugClassLoader with these libraries.

This is for example the case in Drupal 馃槩 (cf https://github.com/drupal/core/blob/83bc30ac4030ed163e1919ca5e12b338a22c87dd/lib/Drupal/Component/ClassFinder/ClassFinder.php).

Fixing these bad implementations in third party libraries can take forever as things move way slower than in Symfony. This is why I think supporting this case directly in Symfony is better. It's easy and will make the DebugClassLoader compatible with more cases.

What could be done to go further in this direction would be to proxify any method implementend by wrapped class loaders.

@fancyweb

This comment has been minimized.

Copy link
Contributor

fancyweb commented Jan 10, 2019

It could be considered as a bug fix as well.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 10, 2019

Is it common to use DebugClassLoader within Drupal?

@fancyweb

This comment has been minimized.

Copy link
Contributor

fancyweb commented Jan 10, 2019

Is it common to use DebugClassLoader within Drupal?

I don't know. I want to use it. A lot of people might currently use it and have not detected this problem because the incriminated class (ClassFinder) is only used in one core module.

But I don't want this PR to be treated as a request for Drupal. I will report the bad implementation to Drupal issues. It just gonna take forever before getting fixed.

The Debug component is here for a better DX. So it just seems logical to try our best in the DebugClassLoader to handle as many cases as possible when they don't overcomplicate things too much or have impacts on the added behaviors.

WDYT of my proposition to try to proxify every wrapped class loader methods ?

@fancyweb fancyweb force-pushed the fancyweb:readd-find-file-in-debug-class-loader branch from 32cfe3f to 547b762 Jan 11, 2019

@nicolas-grekas nicolas-grekas added this to the next milestone Jan 11, 2019

@nicolas-grekas nicolas-grekas changed the title [DebugClassLoader] Readd findFile() method [DebugClassLoader] expose proxyfied findFile() method Jan 11, 2019

@fabpot

fabpot approved these changes Jan 13, 2019

Copy link
Member

fabpot left a comment

as a bug fix to 3.4

@fabpot fabpot changed the base branch from master to 3.4 Jan 13, 2019

@fabpot fabpot force-pushed the fancyweb:readd-find-file-in-debug-class-loader branch from 547b762 to 4f690a3 Jan 13, 2019

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Jan 13, 2019

Thank you @fancyweb.

@fabpot fabpot merged commit 4f690a3 into symfony:3.4 Jan 13, 2019

0 of 3 checks passed

fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

fabpot added a commit that referenced this pull request Jan 13, 2019

bug #29833 [DebugClassLoader] expose proxyfied findFile() method (fan鈥
鈥yweb)

This PR was submitted for the master branch but it was merged into the 3.4 branch instead (closes #29833).

Discussion
----------

[DebugClassLoader] expose proxyfied findFile() method

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | -
| License       | MIT
| Doc PR        | -

As bad as it is, some third party libraries expect that at least one autoload function will be the Composer one and have behaviors that relies on the public `findFile` method.

When the `DebugClassLoader` wraps the Composer `ClassLoader`, the function `findFile` is currently lost. So it becomes impossible to use the `DebugClassLoader` with these libraries.

This is for example the case in Drupal 馃槩 (cf https://github.com/drupal/core/blob/83bc30ac4030ed163e1919ca5e12b338a22c87dd/lib/Drupal/Component/ClassFinder/ClassFinder.php).

Fixing these bad implementations in third party libraries can take forever as things move way slower than in Symfony. This is why I think supporting this case directly in Symfony is better. It's easy and will make the `DebugClassLoader` compatible with more cases.

What could be done to go further in this direction would be to proxify any method implementend by wrapped class loaders.

Commits
-------

4f690a3 [DebugClassLoader] Readd findFile() method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment