Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Config][DependencyInjection] Optimize dumped resources for tracking#57948
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedAug 9, 2024
The failures look related to the changes. |
9fa19e3 toa29fb1eComparenicolas-grekas commentedAug 12, 2024
Yep, fixed |
4b3dc48 to07d646dComparesrc/Symfony/Component/Config/Resource/ReflectionClassResource.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
… on cache clear, impossible to install new project (eltharin)This PR was merged into the 7.2 branch.Discussion----------[DependencyInjection] Fix bug on windows, memory exhausted on cache clear, impossible to install new project| Q | A| ------------- | ---| Branch? | 7.2| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | Fix| License | MITSince#57948 on windows, I can't make a cache clear with a fatal error form memory exhausted or install a new symfony app.Xdebug saw an infinite loop.Correction with DIRECTORY_SEPARATOR constant instead /Commits-------89b4c87 bug correction
| continue; | ||
| } | ||
| if (!$inVendor =$this->inVendors($dir)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This logic looks wrong to me. Configuring a namespace prefixed to match a given directory does not mean that all classes matching that prefix must be defined in that folder.
For instance, all our mailer bridges have a namespace prefix that can also be matched by the prefix of thesymfony/mailer package. And nothing prevents the project-level code to also define classes with that namespace (best practices is to keep your project-level code in a different namespace than your vendors, but that's not a requirement, and I know some projects do weird things).
For existing classes, we can optimize the class existence resource if the class is defined in the vendor folder (by checking the actual class definition, not folders in which it ismaybe defined). For non-existent classes, we should not optimize the resource (or we should optimize it only based onall possible autoloading locations)
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
GromNaN commentedSep 3, 2025
After reading the code, I don't understand which new classes are excluded. |
…peMC)This PR was merged into the 7.3 branch.Discussion----------[Config] Fix `ReflectionClassResource` hash validation| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | -| License | MITIt looks like#57948 broke hash validation.The problem is that `$excludedVendors` is now used to generate the hash but is not serialized. After unserialization, the property is empty, so the generated hash never matches the dumped one.Commits-------cec2f00 [Config] Fix `ReflectionClassResource` hash validation
stof commentedSep 3, 2025
@GromNaN this is about skipping FileResource, DirectoryResource, FileExistenceResource and ClassExistenceResource for resources that are in the This is all based on the assumption that you don't edit manually files in the |
…e (nicolas-grekas)This PR was merged into the 7.3 branch.Discussion----------[DependencyInjection] Fix optimizing ClassExistenceResource| Q | A| ------------- | ---| Branch? | 7.3| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues | -| License | MITPer `@stof` review at#57948 (review)Best reviewed [ignoring whitespaces](https://github.com/symfony/symfony/pull/61615/files?w=1).Commits-------b7ca948 [DependencyInjection] Fix optimizing ClassExistenceResource
Uh oh!
There was an error while loading.Please reload this page.
Looking at the resource tracking cache, I figured out we could remove tracking some more resources.
On a webapp skeleton, this goes from 85 to 39 resources to track.
On my Ubuntu, I couldn't measure a difference, but on slow filesystems (Windows, shared mounts), I hope this might improve the DX a bit during dev. Anyone able to confirm?