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
Appearance settings

[Config] Fixed issue between ComposerResource and symfony/phpunit-bridge #40092

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Config] Fixed issue between ComposerResource and symfony/phpunit-bridge #40092

wants to merge 1 commit into from

Conversation

lyrixx
Copy link
Member

@lyrixx lyrixx commented Feb 4, 2021

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets -
License MIT
Doc PR -

We using symfony/phpunit-bridge package, it installs a new
package in vendor/bin/.phpunit. This package comes with its
own autoloader. When booting symfony with simple-phpunit, the
autoloader is used and discovered here. But we do not want to
affect the container freshness for this package.

@carsonbot carsonbot added this to the 4.4 milestone Feb 4, 2021
@lyrixx lyrixx requested a review from nicolas-grekas February 4, 2021 13:31
@nicolas-grekas
Copy link
Member

What effect does this have in practice?

@lyrixx
Copy link
Member Author

lyrixx commented Feb 4, 2021

the cache is not fresh, so the container keep rebuilding.

With this patch:

Testing Project Test Suite                                                                                                                                                
.♻.♻.♻.♻.                                                               5 / 5 (100%)♻                                                                                     
                                                                                                                                                                          
Time: 4.99 seconds, Memory: 70.50 MB

Without this patch:

Testing Project Test Suite                                                                                                                                                
.♻.♻.♻.♻.                                                               5 / 5 (100%)♻                                                                                     
                                                                                                                                                                          
Time: 1.76 minutes, Memory: 222.50 MB

I forgot to mention something: The client had BIG performance issue because in functional tests they are using this kind of code `bin/console load-fixture`
So the container keeps rebuilding between a functional test, the load of fixture, another functional test, another load of fixtures, etc...

j0k3r
j0k3r reviewed Feb 4, 2021
View reviewed changes
src/Symfony/Component/Config/Resource/ComposerResource.php Outdated Show resolved Hide resolved
@lyrixx lyrixx force-pushed the config-composer-phpunit branch from b2fd8b0 to 3295b10 Compare February 4, 2021 14:02
@nicolas-grekas
Copy link
Member

I feel like the current patch fixes a symptom rather than a cause. Why doesn't ComposerResource work here?

@lyrixx
Copy link
Member Author

lyrixx commented Feb 4, 2021 •
edited
Loading

I'll try to represent what's going on:

$ bin/simple-phpunit
    MyTest
        testFoo
            static::bootKernel() // First boot with simple-phpunit autoloader + regular one
            exec('bin/console') // Second kernel::boot() but **without** the simple-phpunit autoloader => ComposerResource thinks vendors have changed ==> container is rebuilt
        testBar
            static::bootKernel() // Third boot with simple-phpunit autoloader + regular one => ComposerResource thinks vendors have changed ==> container is rebuilt
            exec('bin/console') // Forth kernel::boot() but **without** the simple-phpunit autoloader => ComposerResource thinks vendors have changed ==> container is rebuilt

Is it more clear?

@stof
Copy link
Member

stof commented Feb 4, 2021

@lyrixx this could happen with any tool which is installed as a separate installation than your project vendor folder while also relying on a composer autoloader. this is not specific to the phpunit-bridge.

@lyrixx
Copy link
Member Author

lyrixx commented Feb 4, 2021 •
edited
Loading

What about using $kernel->getProjectDir()/vendor/installed.json ?
If one use something else than vendor for it's vendor 1/ WHY ? 2/ too bad for they

@nicolas-grekas nicolas-grekas mentioned this pull request Feb 4, 2021
Merged
@nicolas-grekas
Copy link
Member

I propose #40098 instead.

nicolas-grekas added a commit that referenced this pull request Feb 5, 2021
@nicolas-grekas
bug #40098 [DependencyInjection] fix tracking of changes to vendor/ d…
d6219f6 
…irs (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] fix tracking of changes to vendor/ dirs

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #40092
| License       | MIT
| Doc PR        | -

Commits
-------

6867475 [DI] fix tracking of changes to vendor/ dirs
@lyrixx lyrixx deleted the config-composer-phpunit branch February 5, 2021 21:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@j0k3r j0k3r j0k3r left review comments

@nicolas-grekas nicolas-grekas Awaiting requested review from nicolas-grekas

Assignees
No one assigned
Labels
Bug Config Status: Needs Review
Projects
None yet
Milestone
4.4
Development

Successfully merging this pull request may close these issues.
5 participants
@lyrixx @nicolas-grekas @stof @j0k3r @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.