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

Fixed composer resources between web/cli #23213

Merged
merged 1 commit into from
Jun 16, 2017
Merged

Fixed composer resources between web/cli #23213

merged 1 commit into from
Jun 16, 2017

Conversation

linaori
Copy link
Contributor

@linaori linaori commented Jun 16, 2017

Q A
Branch? 3.3
Bug fix? yes
New feature? no (reverts one)
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #23212
License MIT
Doc PR ~

This is a possible fix for the flawed module check for the composer resource. As this is the easiest fix, I've created a PR ready to be merged.

@stof
Copy link
Member

stof commented Jun 16, 2017

Well, if we have some configuration set based on the presence of an extension (or the existence of a class provided by an extension), this would prevent purging the cache when needed

@linaori
Copy link
Contributor Author

linaori commented Jun 16, 2017

I know, but I also consider this an edge-case. When you enable a module, it makes sense to use a clear cache. If your feature doesn't work after enabling the module, first thing you should do: clear your cache.

Perhaps this check can be done elsewhere (WDT?) and show that the modules have changed between 2 requests.

@stof
Copy link
Member

stof commented Jun 16, 2017

but if the cache was created in the CLI assuming the extension was there, and there you PHP-FPM process does not have it, your project is broken too

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

let's do that, was already the case in 3.2 and before

@nicolas-grekas nicolas-grekas added this to the 3.3 milestone Jun 16, 2017
@linaori
Copy link
Contributor Author

linaori commented Jun 16, 2017

I don't have have PHP-FPM and my project is not broken in the web because it doesn't have PCNTL. In this case, not having PCNTL in the web, is by design.

Another solution that I've discussed with colleagues here, is making it configurable via possibly a composer extra setting.

@yannickl88
Copy link

@stof, are you aware this is broken now for every setup where the CLI and Web extensions differ? (for instance, not having an apachehandler in CLI makes sense, since you do not need it).

The current behavior triggers a container rebuild for every request for no reason.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 16, 2017

I don't get why you say this is triggering a rebuild for every request.
Only the first web request should do it, then the next ones should be "fresh" from this resource's pov, isn't it?

@linaori
Copy link
Contributor Author

linaori commented Jun 16, 2017

@nicolas-grekas That's what I expected, but is not the case, most likely because we run commands on the CLI during cache warmup.

But even in this case, calling the console would again trigger a rebuild, switching back to the web, another one.

@yannickl88
Copy link

yannickl88 commented Jun 16, 2017

@nicolas-grekas ah yes, what @iltar says.

During a request when the cache needs a warmup we run a CLI command, which triggers this bug again and rebuilds the container again for CLI. So at the end of that request we are back in "CLI-config" so the next request will do the same rebuild. Hence why it happens with every request.

@nicolas-grekas
Copy link
Member

Let's do that and maybe add another more fine grained resource in 3.4 if we want to - tracking only the actually used extensions.

@fabpot
Copy link
Member

fabpot commented Jun 16, 2017

Thank you @iltar.

@fabpot fabpot merged commit 9e04712 into symfony:3.3 Jun 16, 2017
fabpot added a commit that referenced this pull request Jun 16, 2017
This PR was merged into the 3.3 branch.

Discussion
----------

Fixed composer resources between web/cli

| Q             | A
| ------------- | ---
| Branch?       | 3.3
| Bug fix?      | yes
| New feature?  | no (reverts one)
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #23212
| License       | MIT
| Doc PR        | ~

This is a possible fix for the flawed module check for the composer resource. As this is the easiest fix, I've created a PR ready to be merged.

Commits
-------

9e04712 Fixed composer resources between web/cli
@fabpot fabpot mentioned this pull request Jul 4, 2017
@linaori linaori deleted the fix/composer-resource-checker branch February 8, 2019 13:38
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.

6 participants