10000 Fix tests for EventDispatcher by derrabus · Pull Request #24 · nicolas-grekas/symfony · GitHub
[go: up one dir, main page]

Skip to content

Fix tests for EventDispatcher #24

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.

8000

Already on GitHub? Sign in to your account

Conversation

derrabus
Copy link

Fixes tests for the EventDispatcher component. Contains a cherry-pick of symfony#33299.

# Conflicts:
#	src/Symfony/Component/EventDispatcher/Tests/ImmutableEventDispatcherTest.php
@derrabus derrabus changed the title Return types/event dispatcher Fix tests for EventDispatcher Aug 23, 2019
@nicolas-grekas nicolas-grekas merged commit 4bee65a into nicolas-grekas:eh-return-types Aug 23, 2019
@derrabus derrabus deleted the return-types/event-dispatcher branch August 23, 2019 10:57
nicolas-grekas added a commit to symfony/symfony that referenced this pull request Aug 26, 2019
…CI (nicolas-grekas)

This PR was merged into the 4.4 branch.

Discussion
----------

Add return-types with help from DebugClassLoader in the CI

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #33228
| License       | MIT
| Doc PR        | -

I've spent a great deal of time on this PR, experimenting with adding return types to the codebase.

TL;DR: my conclusion is that we cannot make it for 5.0.

There are two reasons for this:
1. The burden this will put on the community is immense, especially when considering that third party libs must also be updated for any apps to work at all on a return-typed 5.0. Symfony must add them last, not first.
2. We need return type covariance, yet this won't be available before PHP 7.4, while 5.0 supports 7.2.

What's attached?
- ~a draft patching logic in `DebugClassLoader` to add return-type where it discovers this should be done~
- return types added automatically thanks to #33283
- ~manual fixes for situations not handled (yet, if possible at all) by that logic in `DebugClassLoader`~ #33332

What's achieved? Tests are green \o/

At this stage, I think we have to acknowledge we won't add return-types in 5.0 but prepare a serious plan to add them in 6.0.

This plan could be:
- [x] make DebugClassLoader able to automate adding return types.
- [x] in 4.4: add all possible return types that don't break BC, e.g. in `Tests` and in generated code
- [x] spot and fix places where annotations aren't accurate, add more annotations where possible.
- [x] ensure `DebugClassLoader` triggers the best possible deprecations that encourage ppl to add return-types in their libs/apps. This means we could decide to disable the current ones (see #33235) and to re-enable them in 5.1. This will also give us the time to fine-tune the tooling (item 1. on this list)

Ideally, we could reach a point where we could test branch 4.4 *with* return-types: we'd use the tooling to add them automatically in the CI job, then we'd run tests and they should be green. Let's do this?

Help Wanted, here is how:

*With PHP 7.4*, run `php .github/patch-types.php`. This will add return types everywhere possible.

Then run tests, e.g. `./phpunit src/Symfony/Component/HttpFoundation --exclude-group legacy,issue-32995`

Here are the components that fail with return types added, please help me check them all with a PR on [my fork](https://github.com/nicolas-grekas/symfony/tree/eh-return-types):
 - [x] src/Symfony/Bridge/Doctrine
 - [x] src/Symfony/Bridge/Monolog
 - [x] src/Symfony/Bridge/PhpUnit
 - [x] src/Symfony/Bridge/ProxyManager
 - [x] src/Symfony/Bridge/Twig
 - [x] src/Symfony/Bundle/DebugBundle
 - [x] src/Symfony/Bundle/FrameworkBundle
 - [x] src/Symfony/Bundle/SecurityBundle
 - [x] src/Symfony/Bundle/TwigBundle
 - [x] src/Symfony/Bundle/WebProfilerBundle
 - [x] src/Symfony/Bundle/WebServerBundle
 - [x] src/Symfony/Component/Asset
 - [x] src/Symfony/Component/BrowserKit
 - [x] src/Symfony/Component/Cache
 - [x] nicolas-grekas#28 src/Symfony/Component/Config
 - [x] src/Symfony/Component/Console
 - [x] src/Symfony/Component/CssSelector
 - [x] src/Symfony/Component/Debug
 - [x] nicolas-grekas#28 src/Symfony/Component/DependencyInjection
 - [x] src/Symfony/Component/DomCrawler
 - [x] src/Symfony/Component/Dotenv
 - [x] src/Symfony/Component/ErrorHandler
 - [x] src/Symfony/Component/ErrorRenderer
 - [x] nicolas-grekas#24 src/Symfony/Component/EventDispatcher
 - [x] src/Symfony/Component/ExpressionLanguage
 - [x] src/Symfony/Component/Filesystem
 - [x] src/Symfony/Component/Finder
 - [x] src/Symfony/Component/Form
 - [x] src/Symfony/Component/HttpClient
 - [x] src/Symfony/Component/HttpFoundation
 - [x] src/Symfony/Component/HttpKernel
 - [x] src/Symfony/Component/Inflector
 - [x] src/Symfony/Component/Intl
 - [x] src/Symfony/Component/Ldap
 - [x] src/Symfony/Component/Lock
 - [x] src/Symfony/Component/Mailer
 - [x] src/Symfony/Component/Messenger
 - [x] src/Symfony/Component/Mime
 - [x] src/Symfony/Component/OptionsResolver
 - [x] src/Symfony/Component/Process
 - [x] src/Symfony/Component/PropertyAccess
 - [x] src/Symfony/Component/PropertyInfo
 - [x] nicolas-grekas#25 src/Symfony/Component/Routing
 - [x] nicolas-grekas#26 src/Symfony/Component/Security
 - [x] src/Symfony/Component/Security/Core
 - [x] src/Symfony/Component/Security/Guard
 - [x] src/Symfony/Component/Security/Http
 - [x] nicolas-grekas#29 src/Symfony/Component/Serializer
 - [x] src/Symfony/Component/Security/Csrf
 - [x] src/Symfony/Component/Stopwatch
 - [x] src/Symfony/Component/Templating
 - [x] nicolas-grekas#27 src/Symfony/Component/Translation
 - [x] src/Symfony/Component/Validator
 - [x] src/Symfony/Component/VarDumper
 - [x] src/Symfony/Component/VarExporter
 - [x] src/Symfony/Component/WebLink
 - [x] src/Symfony/Component/Workflow
 - [x] src/Symfony/Component/Yaml
 - [x] src/Symfony/Contracts

Commits
-------

11149a1 Add return-types with help from DebugClassLoader in the CI
nicolas-grekas pushed a commit that referenced this pull request Mar 7, 2024
…hen publishing a message. (jwage)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[Messenger] [Amqp] Handle AMQPConnectionException when publishing a message.

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix symfony#36538 Fix symfony#48241
| License       | MIT

If you have a message handler that dispatches messages to another queue, you can encounter `AMQPConnectionException` with the message "Library error: a SSL error occurred" or "a socket error occurred"  depending on if you are using tls or not or if you are running behind a load balancer or not.

You can manually reproduce this issue by dispatching a message where the handler then dispatches another message to a different queue, then go to rabbitmq admin and close the connection manually, then dispatch another message and when the message handler goes to dispatch the other message, you will get this exception:

```
a socket error occurred
#0 /vagrant/vendor/symfony/amqp-messenger/Transport/AmqpTransport.php(60): Symfony\Component\Messenger\Bridge\Amqp\Transport\AmqpSender->send()
#1 /vagrant/vendor/symfony/messenger/Middleware/SendMessageMiddleware.php(62): Symfony\Component\Messenger\Bridge\Amqp\Transport\AmqpTransport->send()
#2 /vagrant/vendor/symfony/messenger/Middleware/FailedMessageProcessingMiddleware.php(34): Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle()
#3 /vagrant/vendor/symfony/messenger/Middleware/DispatchAfterCurrentBusMiddleware.php(61): Symfony\Component\Messenger\Middleware\FailedMessageProcessingMiddleware->handle()
#4 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle()
#5 /vagrant/vendor/symfony/messenger/Middleware/AddBusNameStampMiddleware.php(37): Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware->handle()
#6 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle()
#7 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle()
#8 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch()
#9 /vagrant/src/Messenger/MessageBus.php(37): Symfony\Component\Messenger\TraceableMessageBus->dispatch()
#10 /vagrant/vendor/symfony/mailer/Mailer.php(66): App\Messenger\MessageBus->dispatch()
#11 /vagrant/src/Mailer/Mailer.php(83): Symfony\Component\Mailer\Mailer->send()
#12 /vagrant/src/Mailer/Mailer.php(96): App\Mailer\Mailer->send()
#13 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(118): App\Mailer\Mailer->sendEmail()
#14 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(72): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->handle()
#15 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(152): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->__invoke()
#16 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(91): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->callHandler()
#17 /vagrant/vendor/symfony/messenger/Middleware/SendMessageMiddleware.php(71): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->handle()
#18 /vagrant/vendor/symfony/messenger/Middleware/FailedMessageProcessingMiddleware.php(34): Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle()
#19 /vagrant/vendor/symfony/messenger/Middleware/DispatchAfterCurrentBusMiddleware.php(68): Symfony\Component\Messenger\Middleware\FailedMessageProcessingMiddleware->handle()
#20 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle()
#21 /vagrant/vendor/symfony/messenger/Middleware/AddBusNameStampMiddleware.php(37): Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware->handle()
#22 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle()
#23 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle()
#24 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch()
#25 /vagrant/vendor/symfony/messenger/RoutableMessageBus.php(54): Symfony\Component\Messenger\TraceableMessageBus->dispatch()
#26 /vagrant/vendor/symfony/messenger/Worker.php(162): Symfony\Component\Messenger\RoutableMessageBus->dispatch()
#27 /vagrant/vendor/symfony/messenger/Worker.php(109): Symfony\Component\Messenger\Worker->handleMessage()
#28 /vagrant/vendor/symfony/messenger/Command/ConsumeMessagesCommand.php(238): Symfony\Component\Messenger\Worker->run()
#29 /vagrant/vendor/symfony/console/Command/Command.php(326): Symfony\Component\Messenger\Command\ConsumeMessagesCommand->execute()
#30 /vagrant/vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run()
#31 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(126): Symfony\Component\Console\Application->doRunCommand()
#32 /vagrant/vendor/symfony/console/Application.php(324): Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand()
#33 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(80): Symfony\Component\Console\Application->doRun()
#34 /vagrant/vendor/symfony/console/Application.php(175): Symfony\Bundle\FrameworkBundle\Console\Application->doRun()
#35 /vagrant/vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php(49): Symfony\Component\Console\Application->run()
#36 /vagrant/vendor/autoload_runtime.php(29): Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run()
#37 /vagrant/bin/console(11): require_once('...')
#38 {main}
```

TODO:

- [x] Add test for retry logic when publishing messages

Commits
-------

f123370 [Messenger] [Amqp] Handle AMQPConnectionException when publishing a message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants
0