Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[DebugClassLoader] expose proxyfied findFile() method#29833

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged

Conversation

@fancyweb
Copy link
Contributor

@fancywebfancyweb commentedJan 10, 2019•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets-
LicenseMIT
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 publicfindFile method.

When theDebugClassLoader wraps the ComposerClassLoader, the functionfindFile is currently lost. So it becomes impossible to use theDebugClassLoader with these libraries.

This is for example the case in Drupal 😢 (cfhttps://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 theDebugClassLoader 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
ContributorAuthor

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
ContributorAuthor

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 theDebugClassLoader 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 ?

@fancywebfancywebforce-pushed thereadd-find-file-in-debug-class-loader branch from32cfe3f to547b762CompareJanuary 11, 2019 15:10
@nicolas-grekasnicolas-grekas added this to thenext milestoneJan 11, 2019
@nicolas-grekasnicolas-grekas changed the title[DebugClassLoader] Readd findFile() method[DebugClassLoader] expose proxyfied findFile() methodJan 11, 2019
Copy link
Member

@fabpotfabpot 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

@fabpotfabpot changed the base branch frommaster to3.4January 13, 2019 16:36
@fabpotfabpotforce-pushed thereadd-find-file-in-debug-class-loader branch from547b762 to4f690a3CompareJanuary 13, 2019 16:36
@fabpot
Copy link
Member

Thank you@fancyweb.

@fabpotfabpot merged commit4f690a3 intosymfony:3.4Jan 13, 2019
fabpot added a commit that referenced this pull requestJan 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 😢 (cfhttps://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 referencedJan 29, 2019
@nicolas-grekasnicolas-grekas modified the milestones:next,4.3Apr 30, 2019
@fancywebfancyweb deleted the readd-find-file-in-debug-class-loader branchAugust 9, 2019 07:13
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

+1 more reviewer

@ro0NLro0NLro0NL left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

4.3

Development

Successfully merging this pull request may close these issues.

5 participants

@fancyweb@nicolas-grekas@fabpot@ro0NL@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp