[HttpKernel] Move services reset to Kernel::handle()+boot() #24709
Conversation
fddc8fe to
dcf4e78
Compare
| @@ -186,7 +187,15 @@ public function handle(Request $request, $type = HttpKernelInterface::MASTER_REQ | |||
| $this->boot(); | |||
| } | |||
|
|
|||
| return $this->getHttpKernel()->handle($request, $type, $catch); | |||
| try { | |||
| if (0 === $this->requestStackSize++ && $this->container->has('services_resetter')) { | |||
There was a problem hiding this comment.
Wouldn't it be sufficient to check for a master request?
There was a problem hiding this comment.
Sufficient like calling reset() in kernel.terminate ? ;)
I'm sure there is code out there that would thank us being extra cautious here.
| * @param string $tagName | ||
| */ | ||
| public function __construct($tagName = 'kernel.reset') | ||
| public function __construct($serviceName = 'services_resetter', $tagName = 'kernel.reset') |
There was a problem hiding this comment.
Is there any point about making the service name configurable, whereas it's hardcoded in the Kernel?
There was a problem hiding this comment.
We made other passes configurable, so is this one. Doesn't hurt...
There was a problem hiding this comment.
$serviceId, perhaps even $resetterServiceId. (ref)
dcf4e78 to
a0b1580
Compare
|
comments addressed, and tests green |
| $this->resetMethods = $resetMethods; | ||
| } | ||
|
|
||
| public function resetServices() |
There was a problem hiding this comment.
If the class is called ServicesResetter, shouldn't this method be called just reset() to follow official Symfony naming conventions?
a0b1580 to
d957f79
Compare
|
I have a very minor comment about the naming of |
|
@javiereguiluz I don't understand the consideration. Anyway, that's in the dI subnamespace, where we talk about "services", so I don't think we need to change the name. |
|
Ok now I read the FIG's blog post :) Services is the term we use in Symfony, so the current naming is correct to me. |
| * @author Alexander M. Turek <me@derrabus.de> | ||
| * @author Nicolas Grekas <p@tchwork.com> | ||
| */ | ||
| class ServicesResetter |
There was a problem hiding this comment.
This class should probably be marked as @internal
There was a problem hiding this comment.
internal it is now
d957f79 to
4501a36
Compare
|
Thank you @nicolas-grekas. |
…) (nicolas-grekas) This PR was merged into the 3.4 branch. Discussion ---------- [HttpKernel] Move services reset to Kernel::handle()+boot() | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #24552 | License | MIT | Doc PR | - This is an alternative to #24697 (which uses middlewares). This PR adds a new `services_resetter` service that the Kernel calls on 2nd root requests to reset services. Instead of #24697 which plans for optional enabling of the services reset, this approach moves the responsibility of calling the services resetter to the core Kernel class, so that no configuration/middleware/etc. is required at all, and no overhead exists at all for regular requests. Commits ------- 4501a36 [HttpKernel] Move services reset to Kernel
| private $resettableServices; | ||
| private $resetMethods; | ||
|
|
||
| public function __construct(\Traversable $resettableServices, array $resetMethods) |
There was a problem hiding this comment.
How about having only one argument with an iterator over an array of callables, so that instead of $service->{$this->resetMethods[$id]}(); you'll do $callable(), where $callable is [$service, $resetMethod]
There was a problem hiding this comment.
the iterators build by the container using IteratorArgument are restricted to yielding services only, so that's not possible with the current primitives (and we don't really care, so all is fine :) )
| if ($this->debug && !isset($_SERVER['SHELL_VERBOSITY'])) { | ||
| putenv('SHELL_VERBOSITY=3'); | ||
| $_ENV['SHELL_VERBOSITY'] = 3; | ||
| $_SERVER['SHELL_VERBOSITY'] = 3; |
There was a problem hiding this comment.
May be put this login into DotEnv component?
…lamirault) This PR was squashed before being merged into the 5.4 branch. Discussion ---------- [DependencyInjection] Update kernel.reset explanation Try to fix #16806 Change was done in this PR symfony/symfony#24709 I'm open for better wording 😄 Commits ------- a8803e2 [DependencyInjection] Update kernel.reset explanation
This is an alternative to #24697 (which uses middlewares).
This PR adds a new
services_resetterservice that the Kernel calls on 2nd root requests to reset services.Instead of #24697 which plans for optional enabling of the services reset, this approach moves the responsibility of calling the services resetter to the core Kernel class, so that no configuration/middleware/etc. is required at all, and no overhead exists at all for regular requests.