Ensure all handlers are explicitly closed on kernel shutdown #377
Conversation
andy-educake
force-pushed
the
376-add-handler-manager-to-close-handlers
branch
from
6e7a558
to
5c10161
Compare
There was a problem hiding this comment.
I like this PR, but I'm not sure about it. If we do that, we should do the same thing for all others bundle that does IO.
@nicolas-grekas WDYT?
|
Thanks for the comments @lyrixx Have dealt with all of them I think. I notice that Travis has failed for PHP 5.6 and PHP 7.0. Possibly because I used the |
| */ | ||
| private $handlers; | ||
|
|
||
| public function __construct(\IteratorAggregate $handlers) |
There was a problem hiding this comment.
| public function __construct(\IteratorAggregate $handlers) | |
| public function __construct(iterable $handlers) |
The type IteratorAggregate is a bit more specific that it needs to be, isn‘t it?
There was a problem hiding this comment.
the bundle supports PHP 5.6+ currently
There was a problem hiding this comment.
but Traversable should be used instead of IteratorAggregate
There was a problem hiding this comment.
we should drop support for SF 3.X, and bump PHP to 7.1
IMHO, we must do that before this PR
| @@ -40,5 +40,9 @@ | |||
| <service id="monolog.http_client" class="Symfony\Contracts\HttpClient\HttpClientInterface" public="false"> | |||
| <factory class="Symfony\Component\HttpClient\HttpClient" method="create" /> | |||
| </service> | |||
|
|
|||
| <service id="monolog.handler_manager" class="Symfony\Bundle\MonologBundle\HandlerLifecycleManager" public="true"> | |||
| <argument type="tagged_iterator" tag="monolog.handler" /> | |||
There was a problem hiding this comment.
Rather than using type="tagged_iterator", you should use a compiler pass to create the IteratorArgument (or drop the tag entirely and manage everything in the DI extension as I don't think we need to support handlers added elsewhere). This way, you can use weak references in the IteratorArgument, which will skip services which haven't been instantiated yet
There was a problem hiding this comment.
I'm not sure I'm familiar enough with the Container to change this right now but given #377 (comment) I should probably pause on this PR now anyway?
|
Regarding a generic solution in symfony, I think we need something similar to the |
|
|
||
| use Monolog\Handler\HandlerInterface; | ||
|
|
||
| class HandlerLifecycleManager |
There was a problem hiding this comment.
should be tagged as @internal. We really don't want to expose it in the BC promise.
No description provided.