Handlers should be lazy loaded #3

mvrhov · 2017-12-25T07:15:06Z

No description provided.

sroze · 2017-12-25T10:56:55Z

src/Symfony/Component/Message/HandlerLocator.php

@@ -19,15 +20,19 @@
 */
 class HandlerLocator implements HandlerLocatorInterface
 {
+    /** @var ContainerInterface */


Either on 3 lines or to be removed :)

sroze · 2017-12-25T10:57:25Z

src/Symfony/Component/Message/HandlerLocator.php

-            $handler = new MessageHandlerCollection($handler);
+
+        if (is_array($handler)) {
+            $handler = new MessageHandlerCollection(array_map(function ($handler) { return $this->serviceLocator->get($handler); }, $handler));


Could you add the callable's body on its own line?

sroze · 2017-12-25T10:58:47Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/message.xml

@@ -20,6 +20,7 @@

        <!-- Handlers -->
        <service id="message.handler_resolver" class="Symfony\Component\Message\HandlerLocator">
+            <argument type="service" id="service_container"/>


We actually do not inject the service_container, right? So:

<argument />

ro0NL · 2017-12-25T11:12:35Z

src/Symfony/Component/Message/HandlerLocator.php

    /**
     * Maps a message (its class) to a given handler.
     *
     * @var array
     */
    private $messageToHandlerMapping;

-    public function __construct(array $messageToHandlerMapping = array())
+    public function __construct(ContainerInterface $serviceLocator, array $messageToHandlerMapping = array())


Cant the locator do the mapping as well? $handler = $locator->get(get_class($message))

Meaning that multiple handlers per message are normalized to a chain/collection handler at compile time.

mvrhov · 2017-12-25T18:59:22Z

src/Symfony/Component/Message/DependencyInjection/MessagePass.php

@@ -79,7 +81,22 @@ private function findHandlers(ContainerBuilder $container): array
            $handlersByMessage[$message] = call_user_func_array('array_merge', $handlersByMessage[$message]);
        }

-        return $handlersByMessage;
+
+        $definitions = array();


@ro0NL is this what you had in mind

Yep, exactly.

mvrhov · 2017-12-25T19:01:00Z

src/Symfony/Component/Message/DependencyInjection/MessagePass.php

+            } else {
+                $d = new Definition(MessageHandlerCollection::class, array($handlers));
+                $d->setPrivate(true);
+                $definitions[hash('sha1', $message)] = $d;


Do we need to prefix the service name or are we sure that sha1 is unique enough?
If we go for prefix. Suggestions. I'd add MHC short for MessageHandlerCollection

Why not MessageHandlerCollection ? :-)

The prefix? I find it rather long 40 characters of sha1 + 24 for prefix. We'll have problems on windows when this gets dumped to filesystem with the new container (aka service per file) (Too long path)

Fair enough, let's start with that and then we'll see the reviews in Symfony's main PR anyway :)

mvrhov · 2017-12-25T19:02:20Z

src/Symfony/Component/Message/Handler/MessageHandlerCollection.php

-        foreach ($this->handlers as $handler) {
-            yield $handler($message);
-        }
+        return array_map(function ($handler) use ($message) {


@sroze: Sorry I had to revert this, otherwise the generator was returned from this and no handler was called.

Oh... I see, good point! Good reason to revert it 👍

FYI I've merged this change already in the main branch.

sroze · 2017-12-25T19:21:58Z

src/Symfony/Component/Message/DependencyInjection/MessagePass.php

+            } else {
+                $d = new Definition(MessageHandlerCollection::class, array($handlers));
+                $d->setPrivate(true);
+                $definitions[hash('sha1', $message)] = $d;


Why not MessageHandlerCollection ? :-)

sroze · 2017-12-25T19:22:25Z

src/Symfony/Component/Message/Handler/MessageHandlerCollection.php

-        foreach ($this->handlers as $handler) {
-            yield $handler($message);
-        }
+        return array_map(function ($handler) use ($message) {


Oh... I see, good point! Good reason to revert it 👍

sroze · 2017-12-25T19:23:23Z

src/Symfony/Component/Message/HandlerLocator.php

 use Symfony\Component\Message\Exception\NoHandlerForMessageException;
-use Symfony\Component\Message\Handler\MessageHandlerCollection;

 /**
 * @author Samuel Roze <samuel.roze@gmail.com>
 */
 class HandlerLocator implements HandlerLocatorInterface


Can you then rename to ContainerHandlerLocator ?

sroze · 2017-12-25T19:24:21Z

src/Symfony/Component/Message/DependencyInjection/MessagePass.php

+                $d = new Definition(MessageHandlerCollection::class, array($handlers));
+                $d->setPrivate(true);
+                $definitions[hash('sha1', $message)] = $d;
+                $handlersByMessage[$message] = new Reference(hash('sha1', $message));


Can you re-use the hash generated the line above instead of re-generating it?

mvrhov · 2017-12-26T07:07:44Z

I restored the original HandlerLocator. It might come handy to someone.

mvrhov · 2017-12-26T11:45:39Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/message.xml

@@ -7,20 +7,20 @@
    <services>
        <defaults public="false" />

-        <!-- Bus -->
+        <!-- Message Bus -->


on purpose, in preparation for event bus

mvrhov · 2017-12-26T11:46:08Z

src/Symfony/Bundle/FrameworkBundle/Resources/config/message.xml

        <service id="message_bus" class="Symfony\Component\Message\MessageBus" public="true">
            <argument type="collection" /> <!-- Middlewares -->
        </service>

        <service id="Symfony\Component\Message\MessageBusInterface" alias="message_bus" />

        <!-- Handlers -->
-        <service id="message.handler_resolver" class="Symfony\Component\Message\HandlerLocator">
-            <argument type="collection" /> <!-- Message to handler mapping -->
+        <service id="message.container_message_handler_resolver" class="Symfony\Component\Message\ContainerHandlerLocator">


message on purpose, in preparation for event bus

sroze · 2017-12-27T10:28:58Z

I will remove the changes "in preparation of #4" from this PR, and merge. Thank you @mvrhov 👍 👏

Koc · 2017-12-27T10:36:30Z

src/Symfony/Component/Message/ContainerHandlerLocator.php

+/**
+ * @author Miha Vrhovnik <miha.vrhovnik@gmail.com>
+ */
+class ContainerHandlerLocator implements HandlerLocatorInterface


WDYT about Factory instead of Locator?

A bit late 🙊

that implies renaming the interface to HandlerFactoryInterface right? Tend to agree we're indeed factorizing here :) but current naming is fine also IMHO.

sroze · 2017-12-27T10:40:28Z

Merged in symfony@3847968.

ro0NL · 2017-12-27T10:41:28Z

src/Symfony/Component/Message/ContainerHandlerLocator.php

+     */
+    private $serviceLocator;
+
+    public function __construct(ContainerInterface $serviceLocator)


SF usually calls this one $container

…s not exist (neeckeloo) This PR was merged into the 4.2-dev branch. Discussion ---------- [Messenger] Improved message when handler class does not exist | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? |no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | **Problem:** When defining a non existing messenger handler class in the `services.yml` config file, we encounter this confusing error message: ``` services: App\Handler\NonExistentHandler: tags: [messenger.message_handler] ``` ``` PHP Fatal error: Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: Argument 1 passed to Symfony\Component\Messenger\DependencyInjection\MessengerPass::guessHandledClasses() must be an instance of ReflectionClass, null given, called in /app/vendor/symfony/messenger/DependencyInjection/MessengerPass.php on line 93 in /app/vendor/symfony/messenger/DependencyInjection/MessengerPass.php:189 Stack trace: #0 /app/vendor/symfony/messenger/DependencyInjection/MessengerPass.php(93): Symfony\Component\Messenger\DependencyInjection\MessengerPass->guessHandledClasses(NULL, 'App\\Application...') #1 /app/vendor/symfony/messenger/DependencyInjection/MessengerPass.php(74): Symfony\Component\Messenger\DependencyInjection\MessengerPass->registerHandlers(Object(Symfony\Component\DependencyInjection\ContainerBuilder), Array) #2 /app/vendor/symfony/dependency-injection/Compiler/Compiler.php(95): Symfony\Component\Messenger\DependencyInjection\MessengerPass->process(Object(Symfony\Component\DependencyInjection\ContainerBuilder)) #3 / in /app/vendor/symfony/messenger/DependencyInjection/MessengerPass.php on line 189 ``` **Proposal:** We can throw a more relevant exception (RuntimeException) in this case to help the developer to have a better understanding of the issue. ```Invalid service "App\Handler\NonExistentHandler": class "App\Handler\NonExistentHandler" does not exist.``` Commits ------- 6ab9274 Improve error message when defining messenger handler class that does not exists

…logs (fabpot) This PR was merged into the 4.3-dev branch. Discussion ---------- [Messenger] Add missing information in messenger logs | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes-ish | New feature? | yes-ish | BC breaks? | no | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | n/a | License | MIT | Doc PR | n/a When using `messenger:consume`, I get the following logs: ``` 2019-03-25T11:39:05+01:00 [info] Received message "Symfony\Component\Mailer\EnvelopedMessage" 2019-03-25T11:39:05+01:00 [warning] An exception occurred while handling message "Symfony\Component\Mailer\EnvelopedMessage" 2019-03-25T11:39:05+01:00 [info] Retrying Symfony\Component\Mailer\EnvelopedMessage - retry #1. 2019-03-25T11:39:05+01:00 [info] Sending message "Symfony\Component\Mailer\EnvelopedMessage" with "Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransport" 2019-03-25T11:39:06+01:00 [info] Received message "Symfony\Component\Mailer\EnvelopedMessage" 2019-03-25T11:39:06+01:00 [warning] An exception occurred while handling message "Symfony\Component\Mailer\EnvelopedMessage" 2019-03-25T11:39:06+01:00 [info] Retrying Symfony\Component\Mailer\EnvelopedMessage - retry #2. 2019-03-25T11:39:06+01:00 [info] Sending message "Symfony\Component\Mailer\EnvelopedMessage" with "Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransport" 2019-03-25T11:39:09+01:00 [info] Received message "Symfony\Component\Mailer\EnvelopedMessage" 2019-03-25T11:39:09+01:00 [warning] An exception occurred while handling message "Symfony\Component\Mailer\EnvelopedMessage" 2019-03-25T11:39:09+01:00 [info] Retrying Symfony\Component\Mailer\EnvelopedMessage - retry #3. 2019-03-25T11:39:09+01:00 [info] Sending message "Symfony\Component\Mailer\EnvelopedMessage" with "Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransport" ``` So, an. error occurred, but I have no idea what's going on. The exception is in the context, but the context is not displayed by default. So, this PR fixes it. Commits ------- 20664ca [Messenger] added missing information in messenger logs

@nicolas-grekas

… is empty (yceruto) This PR was merged into the 4.2 branch. Discussion ---------- [HttpKernel] Fix get session when the request stack is empty | Q | A | ------------- | --- | Branch? | 4.2 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT This bug happen behind an exception on a kernel response event, when one collector (e.g. `RequestDataCollector`) is trying to get the request session and the request stack is currently empty. **Reproducer** https://github.com/yceruto/get-session-bug (`GET /`) See logs on terminal: ```bash Apr 15 20:29:03 |ERROR| PHP 2019-04-15T20:29:03-04:00 Call to a member function isSecure() on null Apr 15 20:29:03 |ERROR| PHP PHP Fatal error: Uncaught Symfony\Component\Debug\Exception\FatalThrowableError: Call to a member function isSecure() on null in /home/yceruto/demos/getsession/vendor/symfony/http-kernel/EventListener/SessionListener.php:43 Apr 15 20:29:03 |DEBUG| PHP Stack trace: Apr 15 20:29:03 |DEBUG| PHP #0 /home/yceruto/demos/getsession/vendor/symfony/http-kernel/EventListener/AbstractSessionListener.php(59): Symfony\Component\HttpKernel\EventListener\SessionListener->getSession() Apr 15 20:29:03 |DEBUG| PHP #1 /home/yceruto/demos/getsession/vendor/symfony/http-foundation/Request.php(707): Symfony\Component\HttpKernel\EventListener\AbstractSessionListener->Symfony\Component\HttpKernel\EventListener\{closure}() Apr 15 20:29:03 |DEBUG| PHP #2 /home/yceruto/demos/getsession/vendor/symfony/http-kernel/DataCollector/RequestDataCollector.php(65): Symfony\Component\HttpFoundation\Request->getSession() Apr 15 20:29:03 |DEBUG| PHP #3 /home/yceruto/demos/getsession/vendor/symfony/http-kernel/Profiler/Profiler.php(167): Symfony\Component\HttpKernel\DataCollector\RequestDataCollector->collect(Object(Symfony\Component\HttpFoundation\Request), Object(Symfony\Component\HttpFoundation\Respo in /home/yceruto/demos/getsession/vendor/symfony/http-kernel/EventListener/SessionListener.php on line 43 ``` Friendly ping @nicolas-grekas as author of the previous PR symfony#28244 Commits ------- d62ca37 Fix get session when the request stack is empty

This PR was merged into the 4.3 branch. Discussion ---------- [Messenger] improve logs | Q | A | ------------- | --- | Branch? | 4.3 | Bug fix? | no | New feature? | no  | BC breaks? | no  | Deprecations? | no  | Tests pass? | yes  | Fixed tickets | | License | MIT | Doc PR | The logs are currently very confusing and duplicated: - When handled sync the uncaught error it logged and displayed by the console / http error handler anyway. Currently it the warning is duplicated and useless: ``` 14:26:11 WARNING [messenger] An exception occurred while handling message "{class}": OUCH, THAT HURTS! GO TO MOM! 14:26:11 ERROR [console] Error thrown while running command "{class}". Message: "OUCH, THAT HURTS! GO TO MOM!" In HandleMessageMiddleware.php line 82: [Symfony\Component\Messenger\Exception\HandlerFailedException] OUCH, THAT HURTS! GO TO MOM! ``` - When handling async is was even confusing because the actual error was logged as warning and the retry (which is a good thing) was the error. ``` 13:48:15 WARNING [messenger] An exception occurred while handling message "{class}": OUCH, THAT HURTS! GO TO MOM! 13:48:15 ERROR [messenger] Retrying {class} - retry #1. ``` Now it's must clearer and adds even context like the delay: ``` 16:20:11 ERROR [messenger] Error thrown while handling message {class}. Dispatching for retry #3 using 4000 ms delay. Error: "OUCH, THAT HURTS! GO TO MOM!" ... 16:20:15 CRITICAL [messenger] Error thrown while handling message {class}. Removing from transport after 3 retries. Error: "OUCH, THAT HURTS! GO TO MOM!" ``` Commits ------- 2ac7027 [Messenger] improve logs

sroze reviewed Dec 25, 2017

View reviewed changes

sroze mentioned this pull request Dec 25, 2017

[Messenger] Add a new Messenger component symfony/symfony#24411

Closed

ro0NL reviewed Dec 25, 2017

View reviewed changes

sroze force-pushed the add-message-component branch 4 times, most recently from 2824e9a to af34b5c Compare December 25, 2017 12:16

mvrhov commented Dec 25, 2017

View reviewed changes

sroze requested changes Dec 25, 2017

View reviewed changes

sroze force-pushed the add-message-component branch from 3d4d82b to 7549ea5 Compare December 25, 2017 21:11

Handlers should be lazy loaded

50d25a3

mvrhov commented Dec 26, 2017

View reviewed changes

Koc reviewed Dec 27, 2017

View reviewed changes

sroze closed this Dec 27, 2017

ro0NL reviewed Dec 27, 2017

View reviewed changes

mvrhov deleted the lazyLoadHandlers branch December 27, 2017 10:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handlers should be lazy loaded #3

Handlers should be lazy loaded #3

mvrhov commented Dec 25, 2017

sroze Dec 25, 2017

sroze Dec 25, 2017

sroze Dec 25, 2017

ro0NL Dec 25, 2017 •

edited

Loading

mvrhov Dec 25, 2017

ro0NL Dec 25, 2017

mvrhov Dec 25, 2017

sroze Dec 25, 2017

mvrhov Dec 25, 2017 •

edited

Loading

sroze Dec 25, 2017

mvrhov Dec 25, 2017

sroze Dec 25, 2017

sroze Dec 25, 2017

sroze Dec 25, 2017

sroze Dec 25, 2017

sroze Dec 25, 2017

sroze Dec 25, 2017

mvrhov commented Dec 26, 2017

mvrhov Dec 26, 2017

mvrhov Dec 26, 2017

sroze commented Dec 27, 2017

Koc Dec 27, 2017

sroze Dec 27, 2017

ro0NL Dec 27, 2017 •

edited

Loading

sroze commented Dec 27, 2017

ro0NL Dec 27, 2017

Handlers should be lazy loaded #3

Handlers should be lazy loaded #3

Conversation

mvrhov commented Dec 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Dec 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvrhov Dec 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mvrhov commented Dec 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sroze commented Dec 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ro0NL Dec 27, 2017 • edited Loading

Choose a reason for hiding this comment

sroze commented Dec 27, 2017

Choose a reason for hiding this comment

ro0NL Dec 25, 2017 •

edited

Loading

mvrhov Dec 25, 2017 •

edited

Loading

ro0NL Dec 27, 2017 •

edited

Loading