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

[DebugClassLoader] expose proxyfied findFile() method #29833

Merged
merged 1 commit into from Jan 13, 2019

Conversation

fancyweb
Copy link
Contributor

@fancyweb 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
Copy link
Contributor Author

It could be considered as a bug fix as well.

@nicolas-grekas
Copy link
Member

Is it common to use DebugClassLoader within Drupal?

@fancyweb
Copy link
Contributor Author

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 readd-find-file-in-debug-class-loader branch from 32cfe3f to 547b762 Compare January 11, 2019 15:10
@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
Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a bug fix to 3.4

@fabpot fabpot changed the base branch from master to 3.4 January 13, 2019 16:36
@fabpot fabpot force-pushed the readd-find-file-in-debug-class-loader branch from 547b762 to 4f690a3 Compare January 13, 2019 16:36
@fabpot
Copy link
Member

fabpot commented Jan 13, 2019

Thank you @fancyweb.

@fabpot fabpot merged commit 4f690a3 into symfony:3.4 Jan 13, 2019
fabpot added a commit that referenced this pull request Jan 13, 2019
…cyweb)

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
This was referenced Jan 29, 2019
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
@fancyweb fancyweb deleted the readd-find-file-in-debug-class-loader branch August 9, 2019 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants