forked from symfony/symfony
-
Notifications
You must be signed in to change notification settings - Fork 0
feat: event dispatcher before after #3
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
Open
nikophil
wants to merge
1,197
commits into
6.4
Choose a base branch
from
feat/event-dispatcher-before-after
base: 6.4
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7ef8b6b
to
bec344c
Compare
77bf58a
to
49d0896
Compare
eac96a9
to
eefe1e3
Compare
22ebf2d
to
0138136
Compare
… could be thrown while making error message (weaverryan) This PR was squashed before being merged into the 6.3 branch. Discussion ---------- [AssetMapper] Fixing bug where a circular exception could be thrown while making error message | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | none | Tickets | Fix symfony#51291 | License | MIT | Doc PR | Not needed AssetMapper's `JavaScriptImportPathCompiler` parses import statements. That process is imperfect and will over-match in some cases (e.g. matching `import()` that is commented-out). but that's not a huge issue: any matches are simply added to the importmap and matches for not-found-files are ignored. However, in symfony#51291, we hit a spot where, while trying to improve the log message (`Try adding ".js" to the end of the import - i.e. "%s.js"`), we triggered a circular exception. This PR suppresses that. We may need to improve the parsing logic later to handle more edge-cases, but we'll handle those if/when they come up. Cheers! Commits ------- 63b9635 [AssetMapper] Fixing bug where a circular exception could be thrown while making error message
…rove exception message (ahmedghanem00) This PR was merged into the 6.3 branch. Discussion ---------- [Notifier] [Pushover] Fix invalid method call + improve exception message | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | Current code is calling `getContent()` method on an array instead of the real response object Commits ------- ccafda3 Fix invalid method call + improve exception message
* 6.3: [AssetMapper] Fixing bug where a circular exception could be thrown while making error message Dump Valid constaints on debug command symfony#46544 [DependencyInjection] fix dump xml with array/object/enum default value [HttpFoundation] Add a slightly more verbose comment about a warning on UploadedFile [Messenger] BatchHandlerTrait - fix phpdoc typo [SecurityBundle] Remove unused test files Allow passing an `inline_service` to a `service_locator` [Security] Fix error with lock_factory in login_throttling fix(console): fix section output when multiples section with max height Remove me from CODEOWNERS Fix invalid method call + improve exception message Always return bool from messenger amqp conncetion nack [FrameworkBundle] Fix xsd handle-all-throwables [Console] Fix linewraps in OutputFormatter
This PR was merged into the 6.4 branch. Discussion ---------- [Notifier] fix directory casing | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | Commits ------- 0942d6e fix directory casing
…ezone (valtzu) This PR was merged into the 6.3 branch. Discussion ---------- [Scheduler] Match next run timezone with "from" timezone | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | yes | New feature? | no | Deprecations? | no | License | MIT As discussed in symfony#51651 (comment), when a datetime object is created from unix timestamp, the timezone constructor argument is ignored as demonstrated in https://onlinephp.io/c/b07d1 and also mentioned in [PHP documentation](https://www.php.net/manual/en/datetime.construct.php#refsect1-datetime.construct-parameters): > The $timezone parameter and the current timezone are ignored when the $datetime parameter either is a UNIX timestamp (e.g. `@946684800`) or specifies a timezone (e.g. 2010-01-28T15:00:00+02:00). This change shouldn't break any existing logic, given the places where this time is used already include timezone in the date format string. ### Changes in effect ```diff ------------- ------------------------------------ --------------------------- Message Trigger Next Run ------------- ------------------------------------ --------------------------- - TestMessage PeriodicalTrigger: every 2 seconds 2023-09-16T15:54:46+00:00 + TestMessage PeriodicalTrigger: every 2 seconds 2023-09-16T18:54:46+03:00 ------------- ------------------------------------ --------------------------- ``` Commits ------- 9baf427 Match next run timezone with from timezone
* 6.3: Match next run timezone with from timezone
* 5.4: [String] Update wcswidth data with Unicode 15.1 [FrameworkBundle] no serializer mapping cache in debug mode without enable_annotations [Cache] fix using multiple Redis Sentinel hosts when the first one is not resolvable
* 6.3: [String] Update wcswidth data with Unicode 15.1 [FrameworkBundle] no serializer mapping cache in debug mode without enable_annotations [Cache] fix using multiple Redis Sentinel hosts when the first one is not resolvable
…nd security-http 7.0 (alexander-schranz) This PR was merged into the 6.4 branch. Discussion ---------- Fix incompatibility between security-bundle 6.4 and security-http 7.0 | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> The security-bundle 6.4 is not compatible with security-http 7.0: > PHP Fatal error: Declaration of > `Sulu\Bundle\SecurityBundle\Security\AuthenticationEntryPoint::start(` > `Symfony\Component\HttpFoundation\Request $request, ?Symfony\Component\Security\Core\Exception\AuthenticationException $authException = null)` > must be compatible with > `Symfony\Component\Security\Http\EntryPoint\AuthenticationEntryPointInterface::start(` > `Symfony\Component\HttpFoundation\Request $request, ?Symfony\Component\Security\Core\Exception\AuthenticationException $authException = null): Symfony\Component\HttpFoundation\Response` > in /home/runner/work/sulu/sulu/src/Sulu/Bundle/SecurityBundle/Security/AuthenticationEntryPoint.php on line 25 Commits ------- 2a8f8f8 Fix incompatibility between security-bundle 6.4 and security-http 7.0
Our psalm analysis does not analyse tests or usages of Symfony in other projects, so it reports lots of methods as unused.
This PR was merged into the 6.4 branch. Discussion ---------- Disable the dead code analysis in Psalm | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | n/a | License | MIT Our psalm analysis does not analyse tests or usages of Symfony in other projects, so it reports lots of methods as unused. For most jobs, this does not report anything because of our dynamic baseline meant to report only new things, but it creates confusion when it finds new ones (like in https://github.com/symfony/symfony/actions/runs/6224332561) `findDeadCode` defaults to `false` in Psalm 5.x but our CI installs dev versions and does not restricts the version of Psalm that it installs so it already installs 6.x-dev (i.e. dev-master) where the default config for that is `true`. Finding dead code might be a good default for projects. But for libraries, it is not (at least if you don't also analyse the testsuite to discover usages of tested public APIs) Commits ------- 2658b38 Disable the dead code analysis in Psalm
…tcher::runWithLocale()`'s callback (alexander-schranz) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Translation] Give current locale to `LocaleSwitcher::runWithLocale()`'s callback | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> I think it would not hurt to give the $locale everytime to the callback method. Example usage: ```php $notificationContent = $this->localeSwitcher->runWithLocale($contact->getLocale(), function(string $locale) use ($someEntity): string { $title = $someEntity->getTitle($locale); return $this->twig->render('template.html.twig', ['title' => $title]); }); ``` Alternative solution currently is: ```php $locale = $contact->getLocale(); $notificationContent = $this->localeSwitcher->runWithLocale($locale, function() use ($locale, $someEntity): string { $title = $someEntity->getTitle($locale); return $this->twig->render('template.html.twig', ['title' => $title]); }); ``` So this avoids creating a `$locale` variable which maybe only is necessary inside the callback method. Commits ------- 4c658ce [Translation] Give current locale to locale switcher runWithLocale callback
…Oipnet) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Mime] Allow to add some headers as a strings | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no | Deprecations? | no | Tickets | Fix symfony#51584 | License | MIT When adding a MailboxListHeader header, the constructor expects an array as parameter. To simplify the UX, we now allow you to pass a string to the addHeader method in the case of a MailBoxListHeader. The header value is then converted to an array. Commits ------- 00955d0 [Mime] Allow to add some headers as a strings
…ation:pull` command (syffer) This PR was merged into the 6.4 branch. Discussion ---------- [Translation] Add `--as-tree` option to `translation:pull` command | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes/no | New feature? | yes | Deprecations? | no | Tickets | n/a | License | MIT | Doc PR | todo This PR adds an option `--as-tree` to the command `translation:pull`, like the PR symfony#38393 did it for the command `translation:extract`. This option would ease the use of the command `translation:pull` when the yaml translations are stored as a tree. Without it, the yaml translations are inlined after a `translation:pull`, regardless of whether it was stored as inline or as a tree. To bypass this, you would either have to - re-indent the yaml file yourself after a `translation:pull` - replace one service (e.g. `translation.dumper.yaml`) to forcefully add the option `as_tree` for the yaml file dumper Commits ------- 2f328ad [Translation] Add `--as-tree` option to `translation:pull` command
…inel hosts (digilist) This PR was merged into the 6.4 branch. Discussion ---------- [Messenger] Add support for multiple Redis Sentinel hosts | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | n/a | License | MIT | Doc PR | n/a Similar to symfony#47003 which added support for multiple Redis Sentinel hosts for the Cache component, this PR adds support for multiple Sentinel hosts for the Messenger component. This PR is inspired by the implementation in the cache component and works very similar. A DSN could look like this: `redis:?host[localhost:26377]&host[localhost:26379]&sentinel_master=db`. I changed the Sentinel host environment variable for the ingegration to an invalid host at. As a result I noticed that Relay also fails in such case and so I expanded my earlier changes from symfony#51598 to also ignore unreachable hosts with the Relay extension. Commits ------- 3380518 [Messenger] Add support for multiple Redis Sentinel hosts
…teConfig into arrays (welcoMattic) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [Translation] [Phrase] Refacto ReadConfig and WriteConfig into arrays | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | Fix symfony#49231 | License | MIT | Doc PR | symfony/symfony-docs#... <!-- required for new features --> This PR follows up symfony#49231 which brings a new Translation Provider for [Phrase](https://phrase.com/). But there was a complexity layer about read and write configuration (means API calls options) in the original code. I've moved `ReadConfig` and `WriteConfig` from their own class into `PhraseProvider` properties. Which is easier to manipulate, to initialize and to test. Commits ------- 54b38e7 [Translation] [Phrase] Refacto ReadConfig and WriteConfig into arrays
975e2d6
to
f98a75d
Compare
f98a75d
to
142df43
Compare
nikophil
pushed a commit
that referenced
this pull request
Jan 31, 2024
…read from socket (xdanik) This PR was merged into the 5.4 branch. Discussion ---------- [Mailer] Throw `TransportException` when unable to read from socket | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? |no | Issues | None | License | MIT We are seeing error `fgets(): SSL: Connection reset by peer` multiple times a day from connection to Office 365 SMTP server (smtp.office365.com:587). It's certainly related to some kind of timeout as we are sending emails from long running queue dispatcher and error shows up only occasionally and never with the first message. We are not seeing this issue with any other SMTP server, but we have not tested much past smtp.mandrillapp.com and local MailHog. We have tried adjusting the `$pingThreshold` and `$restartThreshold` options, but without much success (well `$restartThreshold = 1` resolves the issue, but it also forces the transport to close connection after each message). Stack trace: ``` #0 /var/www/vendor/symfony/mailer/Transport/Smtp/Stream/AbstractStream.php(77): fgets(Resource(stream)) #1 /var/www/vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php(315): Symfony\Component\Mailer\Transport\Smtp\Stream\AbstractStream->readLine() #2 /var/www/vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php(181): Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->getFullResponse() #3 /var/www/vendor/symfony/mailer/Transport/Smtp/SmtpTransport.php(140): Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->executeCommand("RSET ", Array(1)) #4 /var/www/vendor/symfony/mailer/Mailer.php(45): Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->send(Object(Symfony\Component\Mime\Email), Null) #5 (our queue dispatcher): Symfony\Component\Mailer\Mailer->send(Object(Symfony\Component\Mime\Email)) ``` App is running on PHP 8.0.28 on Debian Linux x64, Mailer v5.4.22. I would gladly written some tests for this, but I don't know how to simulate calls to low-level stream functions like fgets. Commits ------- 44d5b57 [Mailer] Throw TransportException when unable to read from socket
nikophil
pushed a commit
that referenced
this pull request
May 6, 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() symfony#6 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle() symfony#7 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle() symfony#8 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch() symfony#9 /vagrant/src/Messenger/MessageBus.php(37): Symfony\Component\Messenger\TraceableMessageBus->dispatch() symfony#10 /vagrant/vendor/symfony/mailer/Mailer.php(66): App\Messenger\MessageBus->dispatch() symfony#11 /vagrant/src/Mailer/Mailer.php(83): Symfony\Component\Mailer\Mailer->send() symfony#12 /vagrant/src/Mailer/Mailer.php(96): App\Mailer\Mailer->send() symfony#13 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(118): App\Mailer\Mailer->sendEmail() symfony#14 /vagrant/src/MessageHandler/Trading/StrategySubscriptionMessageHandler.php(72): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->handle() symfony#15 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(152): App\MessageHandler\Trading\StrategySubscriptionMessageHandler->__invoke() symfony#16 /vagrant/vendor/symfony/messenger/Middleware/HandleMessageMiddleware.php(91): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->callHandler() symfony#17 /vagrant/vendor/symfony/messenger/Middleware/SendMessageMiddleware.php(71): Symfony\Component\Messenger\Middleware\HandleMessageMiddleware->handle() symfony#18 /vagrant/vendor/symfony/messenger/Middleware/FailedMessageProcessingMiddleware.php(34): Symfony\Component\Messenger\Middleware\SendMessageMiddleware->handle() symfony#19 /vagrant/vendor/symfony/messenger/Middleware/DispatchAfterCurrentBusMiddleware.php(68): Symfony\Component\Messenger\Middleware\FailedMessageProcessingMiddleware->handle() symfony#20 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle() symfony#21 /vagrant/vendor/symfony/messenger/Middleware/AddBusNameStampMiddleware.php(37): Symfony\Component\Messenger\Middleware\RejectRedeliveredMessageMiddleware->handle() symfony#22 /vagrant/vendor/symfony/messenger/Middleware/TraceableMiddleware.php(40): Symfony\Component\Messenger\Middleware\AddBusNameStampMiddleware->handle() symfony#23 /vagrant/vendor/symfony/messenger/MessageBus.php(70): Symfony\Component\Messenger\Middleware\TraceableMiddleware->handle() symfony#24 /vagrant/vendor/symfony/messenger/TraceableMessageBus.php(38): Symfony\Component\Messenger\MessageBus->dispatch() symfony#25 /vagrant/vendor/symfony/messenger/RoutableMessageBus.php(54): Symfony\Component\Messenger\TraceableMessageBus->dispatch() symfony#26 /vagrant/vendor/symfony/messenger/Worker.php(162): Symfony\Component\Messenger\RoutableMessageBus->dispatch() symfony#27 /vagrant/vendor/symfony/messenger/Worker.php(109): Symfony\Component\Messenger\Worker->handleMessage() symfony#28 /vagrant/vendor/symfony/messenger/Command/ConsumeMessagesCommand.php(238): Symfony\Component\Messenger\Worker->run() symfony#29 /vagrant/vendor/symfony/console/Command/Command.php(326): Symfony\Component\Messenger\Command\ConsumeMessagesCommand->execute() symfony#30 /vagrant/vendor/symfony/console/Application.php(1096): Symfony\Component\Console\Command\Command->run() symfony#31 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(126): Symfony\Component\Console\Application->doRunCommand() symfony#32 /vagrant/vendor/symfony/console/Application.php(324): Symfony\Bundle\FrameworkBundle\Console\Application->doRunCommand() symfony#33 /vagrant/vendor/symfony/framework-bundle/Console/Application.php(80): Symfony\Component\Console\Application->doRun() symfony#34 /vagrant/vendor/symfony/console/Application.php(175): Symfony\Bundle\FrameworkBundle\Console\Application->doRun() symfony#35 /vagrant/vendor/symfony/runtime/Runner/Symfony/ConsoleApplicationRunner.php(49): Symfony\Component\Console\Application->run() symfony#36 /vagrant/vendor/autoload_runtime.php(29): Symfony\Component\Runtime\Runner\Symfony\ConsoleApplicationRunner->run() symfony#37 /vagrant/bin/console(11): require_once('...') symfony#38 {main} ``` TODO: - [x] Add test for retry logic when publishing messages Commits ------- f123370 [Messenger] [Amqp] Handle AMQPConnectionException when publishing a message.
nikophil
pushed a commit
that referenced
this pull request
Aug 29, 2024
…rsimpsons) This PR was merged into the 5.4 branch. Discussion ---------- [Yaml] 🐛 throw ParseException on invalid date | Q | A | ------------- | --- | Branch? | 5.4 <!-- see below --> | Bug fix? | yes | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Issues | None <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead --> | License | MIT (found in symfony-tools/docs-builder#179) When parsing the following yaml: ``` date: 6418-75-51 ``` `symfony/yaml` will throw an exception: ``` $ php main.php PHP Fatal error: Uncaught Exception: Failed to parse time string (6418-75-51) at position 6 (5): Unexpected character in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php:714 Stack trace: #0 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(714): DateTimeImmutable->__construct() #1 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(312): Symfony\Component\Yaml\Inline::evaluateScalar() #2 /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php(80): Symfony\Component\Yaml\Inline::parseScalar() #3 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(790): Symfony\Component\Yaml\Inline::parse() #4 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(341): Symfony\Component\Yaml\Parser->parseValue() #5 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(86): Symfony\Component\Yaml\Parser->doParse() symfony#6 /tmp/symfony-yaml/vendor/symfony/yaml/Yaml.php(77): Symfony\Component\Yaml\Parser->parse() symfony#7 /tmp/symfony-yaml/main.php(8): Symfony\Component\Yaml\Yaml::parse() symfony#8 {main} thrown in /tmp/symfony-yaml/vendor/symfony/yaml/Inline.php on line 714 ``` This is because the "month" is invalid. Fixing the "month" will trigger about the same issue because the "day" would be invalid. With the current change it will throw a `ParseException`. Commits ------- 6d71a7e 🐛 throw ParseException on invalid date
nikophil
pushed a commit
that referenced
this pull request
Dec 6, 2024
… not throw exception (lyrixx) This PR was merged into the 5.4 branch. Discussion ---------- [HttpKernel] Ensure `HttpCache::getTraceKey()` does not throw exception | Q | A | ------------- | --- | Branch? | 5.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Issues | - | License | MIT We have such logs in our logs. It's in our raw PHP logs. They are not caught by monolog, it's too early ``` [11-Oct-2024 01:23:33 UTC] PHP Fatal error: Uncaught Symfony\Component\HttpFoundation\Exception\SuspiciousOperationException: Invalid method override "__CONSTRUCT". in /var/www/redirection.io/backend/blue/vendor/symfony/http-foundation/Request.php:1234 Stack trace: #0 /var/www/redirection.io/backend/blue/vendor/symfony/http-kernel/HttpCache/HttpCache.php(728): Symfony\Component\HttpFoundation\Request->getMethod() #1 /var/www/redirection.io/backend/blue/vendor/symfony/http-kernel/HttpCache/HttpCache.php(207): Symfony\Component\HttpKernel\HttpCache\HttpCache->getTraceKey() #2 /var/www/redirection.io/backend/blue/vendor/symfony/http-kernel/Kernel.php(188): Symfony\Component\HttpKernel\HttpCache\HttpCache->handle() #3 /var/www/redirection.io/backend/blue/web/app.php(9): Symfony\Component\HttpKernel\Kernel->handle() #4 {main} thrown in /var/www/redirection.io/backend/blue/vendor/symfony/http-foundation/Request.php on line 1234 ``` I managed to reproduced locally. * Before the patch, without the http_cache, symfony returns a 405 * After the patch, without the http_cache, symfony returns a 405 * Before the patch, with the http_cache, symfony returns a 500, without any information (too early) * After the patch, with the http_cache, symfony returns a 405 Commits ------- a2ebbe0 [HttpKernel] Ensure HttpCache::getTraceKey() does not throw exception
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When hooking into the event system, it's a common need to define the priority compared to another existing listener:
This is not optimal: without the comment it is not clear why the listener has the given priority, and this is not enforced by code.
I'd like to propose another approach, and introduce a new way to handle priorities between listeners:
before
andafter
priority should follow these rules:serviceId
can be providedThe idea here is to rely on the priority system and to guess the priority of the listener based on its "before" or "after" attributes (the priority computation would be recursive if the given before/after listener should also occur before/after another one).
In the proposed implementation, the priorities for all listeners using before/after are computed in
RegisterListenersPass
before we loop over the listeners.Here are some rules:
Here is a first draft PR of the implementation, it's missing a lot of stuff:
at this point, I'd like to have early feedback about the idea and if it has some chances to be accepted. and if the given implementation is the way to go?
I think if this feature is accepted, it would be nice to commit this feature to all tags using priorities, and then add it to
PriorityTaggedServiceTrait
thanks for your feedbacks!