Skip to content

Reimplement Lock & Session using only ext-mongodb #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

Closed
wants to merge 57 commits into from
Closed

Conversation

GromNaN
Copy link
Owner

@GromNaN GromNaN commented Oct 21, 2023

mongodb/mongodb is complex to handle for library with optional support of MongoDB, as it requires ext-mongodb.

In order to reduce complexity for maintainers, this are reimplemented using only the C driver features.

Some features of the client may be missing (server selection, session, transaction). But I don't think they are necessary to session and lock.

Changes:

  • Lock & Session accept a MongoDB\Driver\Manager
  • The lock use exclusively UTC date. Before, there was a mix of time() and UTCDatetime objects.
  • Session tests require a mongodb server.

@GromNaN GromNaN marked this pull request as draft October 21, 2023 00:28
crownbackend and others added 3 commits October 27, 2023 09:38
…xembourgish (crownbackend)

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

Discussion
----------

[Security][Validator] Missing translations for Luxembourgish

| 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        | Fix symfony#51899 <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

Added missing translations validator (translation units 100-109).
Added missing translations security (translation units 10-20).
<!--
Replace this notice by a description of your feature/bugfix.
This will help reviewers and should be a good start for the documentation.

Additionally (see https://symfony.com/releases):
 - Always add tests and ensure they pass.
 - Bug fixes must be submitted against the lowest maintained branch where they apply
   (lowest branches are regularly merged to upper ones so they get the fixes too).
 - Features and deprecations must be submitted against the latest branch.
 - For new features, provide some code snippets to help understand usage.
 - Changelog entry should follow https://symfony.com/doc/current/contributing/code/conventions.html#writing-a-changelog-entry
 - Never break backward compatibility (see https://symfony.com/bc).
-->

Commits
-------

d2518d2 [Security][Validator] Missing translations for Luxembourgish

return true;
return $this->manager->executeBulkWrite($this->namespace, $write)->isAcknowledged();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isAcknowledged() will always return true unless you set the writeConcern option to 0 (either as a bulk write option or as a connection option). If you're looking to return true if the operation succeeded, you can safely leave the previous return statement in place as the operation will throw an exception in case of an error.

Note that a false value for isAcknowledged() doesn't mean that the operation failed - it merely means that the driver has no way of knowing whether the operation succeeded or not.

throw new InvalidArgumentException(sprintf('"%s()" requires the "collection" in the URI querystring or option.', __METHOD__));
}
if (null === $this->options['database']) {
throw new InvalidArgumentException(sprintf('"%s()" requires the "database" in the URI path or option.', __METHOD__));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these extracted from the connection string somehow? I'll note that collection and database are not valid URI options, so unless they are removed from the connection string there are potential problems down the road as the driver may reject unknown URI options in the future.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The database name is extracted from the URI path. The collection name comes from the query string, but it is removed before the URI is given to MongoDB Manager.

cedric-anne and others added 7 commits October 27, 2023 15:27
This PR was merged into the 5.4 branch.

Discussion
----------

[DoctrineBridge] Fix exception message

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | --
| License       | MIT

It is the third, not the second argument.

Commits
-------

5b4961f [DoctrineBridge] Fix exception message
…e (cedric-anne)

This PR was merged into the 6.3 branch.

Discussion
----------

[HtmlSanitizer] Consider `width` attribute as safe

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix symfony#50153
| License       | MIT

Consider the HTML attribute `width` to be safe, as attribute `height` already is.

Commits
-------

827bd6a [HtmlSanitizer] Consider `width` attribute as safe
This PR was merged into the 6.3 branch.

Discussion
----------

[Scheduler] Use MockClock

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Issues        | n/a
| License       | MIT

Commits
-------

6085fe0 [Scheduler] Use MockClock
@GromNaN GromNaN force-pushed the ext-mongodb2 branch 4 times, most recently from e08608a to 11196f1 Compare October 27, 2023 22:01
jderusse and others added 13 commits October 28, 2023 11:19
…intr)

This PR was merged into the 5.4 branch.

Discussion
----------

[Form] Fix merging form data and files (ter)

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix symfony#52318
| License       | MIT

Adapted from a patch provided by `@jan`-pintr

The original was like below but this one looks more appropriate to me.

<details>

```diff
--- a/src/Symfony/Component/Form/Util/FormUtil.php
+++ b/src/Symfony/Component/Form/Util/FormUtil.php
@@ -47,8 +47,12 @@ public static function isEmpty(mixed $data): bool
     public static function mergeParamsAndFiles(array $params, array $files): array
     {
         if (array_is_list($files)) {
-            foreach ($files as $value) {
-                $params[] = $value;
+            foreach ($files as $key => $value) {
+                if (is_array($params[$key])) {
+                    $params[$key] = array_merge($params[$key], $value);
+                } else {
+                    $params[] = $value;
+                }
             }

             return $params;
@@ -61,6 +65,6 @@ public static function mergeParamsAndFiles(array $params, array $files): array
             }
         }

-        return array_replace($params, $files);
+        return array_merge($params, $files);
     }```

</details>

Commits
-------

9facb2f [Form] Fix merging form data and files (ter)
This PR was merged into the 5.4 branch.

Discussion
----------

[Intl] Update the ICU data to 74.1

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | -
| License       | MIT
| Doc PR        | -

Upgrade INTL data to ICU 74.1

Commits
-------

0b09455 [Intl] Update the ICU data to 74.1
* 5.4:
  [Form] Fix merging form data and files (ter)
  [Intl] Update the ICU data to 74.1
  [DoctrineBridge] Fix exception message
  [Security][Validator] Missing translations for Luxembourgish
* 6.3:
  [Form] Fix merging form data and files (ter)
  [Intl] Update the ICU data to 74.1
  [Scheduler] Use MockClock
  [HtmlSanitizer] Consider `width` attribute as safe
  [DoctrineBridge] Fix exception message
  [Security][Validator] Missing translations for Luxembourgish
…lesystem (weaverryan)

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

Discussion
----------

[AssetMapper] Fix in-file imports to resolve via filesystem

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | None
| License       | MIT

Hi!

Another fix from further testing in the complex world!

`tl;dr` Relative imports in CSS & JS files are now resolved via the filesystem path, not the AssetMapper "logical path".

**Full Explanation**

In AssetMapper, we configure different paths/directories, which exposes them to the public directory. If you need to fetch the info about an asset, you can use its "logical path".

However, when you're inside of a JavaScript or CSS file, these "asset mapper" paths or "logical asset paths" are correctly irrelevant. Instead, you should code as if all of your files are already public and won't be moved around:

```js
// /path/to/project/assets/app.js

// (1️⃣) 👍 /path/to/project/assets/files/app.js
import './files/other.js';
// (2️⃣) 👍 /path/to/project/vendor/foo/bar/assets/baz.js
import '../vendor/foo/bar/assets/baz.js

// (3️⃣) 👎
// this is a valid asset mapper "logical path", but this is nonsense and doesn't work
// well, you could add this to the importmap. But "asset mapper" paths still have no relevancy
import 'files/other.js';
```

This PR fixes case 2️⃣ which previously did not work. Before, we used `../vendor/foo/bar/assets/baz.js` and the "asset mapper logical path" of the importing file (`app.js`) and tried to construct a final, logical path from that. But because of the `../vendor`, that went above the `app.js` root, and we panicked. Even if we didn't panic, logical paths should not be relevant here. Instead, it's simpler: the relative import in `app.js` uses the filesystem path. Of course, if the final file - `/vendor/foo/bar/assets/baz.js` is not, itself, in *some* AssetMapper path, an clear exception is thrown (if the file is not exposed publicly, we can't resolve a proper path to it).

Cheers!

Commits
-------

659c635 [AssetMapper] Fix in-file imports to resolve via filesystem
…leNormalizer, add to changelog (xabbuh)

This PR was merged into the 6.4 branch.

Discussion
----------

[Serializer] throw better exception in TranslatableNormalizer, add to changelog

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       |

follows symfony#50212

Commits
-------

d8a03d1 throw better exception in TranslatableNormalizer, add to changelog
…eveloperKid)

This PR was merged into the 5.4 branch.

Discussion
----------

[Yaml] Fix deprecated passing null to trim()

| 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        | Closes symfony#52326 <!-- prefix each issue number with "Fix #", no need to create an issue if none exists, explain below instead -->
| License       | MIT

Commits
-------

54fc3c5 Fix passing null to trim()
* 5.4:
  Fix passing null to trim()
* 6.3:
  Fix passing null to trim()
fabpot and others added 20 commits October 29, 2023 02:07
Incomplete revert in eaff34a following b2e372d and bb5d49c
…NaN)

This PR was merged into the 6.4 branch.

Discussion
----------

[Yaml] Remove test on `Inline::parse(null)`

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

This test was added by symfony#52332
It becomes wrong with symfony#52334
It was not reverted by symfony@eaff34a

Commits
-------

9b10106 Fix wrong yaml parse null test
This PR was merged into the 6.4 branch.

Discussion
----------

[Scheduler] fix test

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

Commits
-------

2f03942 fix test
This PR was merged into the 6.4 branch.

Discussion
----------

[AssetMapper] Fixing merge

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | None
| License       | MIT

Fixing the merge of symfony#52323, symfony#52331 and symfony#52349

Also tested on a real project locally to verify the moving pieces :).

Thanks!

Commits
-------

99d5cbb [AssetMapper] Fixing merge from multiple PR's
…for Response (Hanmac)

This PR was submitted for the 6.4 branch but it was squashed and merged into the 5.4 branch instead.

Discussion
----------

[HttpClient] Psr18Client: parse HTTP Reason Phrase for Response

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

Commits
-------

115a5a1 [HttpClient] Psr18Client: parse HTTP Reason Phrase for Response
* 5.4:
  [HttpClient] Psr18Client: parse HTTP Reason Phrase for Response
  Bump Symfony version to 5.4.31
  Update VERSION for 5.4.30
  Update CONTRIBUTORS for 5.4.30
  Update CHANGELOG for 5.4.30
* 6.3:
  [HttpClient] Psr18Client: parse HTTP Reason Phrase for Response
  Bump Symfony version to 5.4.31
  Update VERSION for 5.4.30
  Update CONTRIBUTORS for 5.4.30
  Update CHANGELOG for 5.4.30
  [Lock] Fix mongodb extension requirement in tests
… by PHP CS Fixer (keradus)

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

Discussion
----------

DX: re-apply self_accessor and phpdoc_types_order by PHP CS Fixer

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        | Fix CS
| License       | MIT

one further step to clean the CS status-quo, aiming for all-green in long-term

Commits
-------

ccd7841 DX: re-apply self_accessor and phpdoc_types_order by PHP CS Fixer
…unctionality is enabled (a.dmitryuk)

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

Discussion
----------

[Process] remove fixing of legacy bug, when PTS functionality is enabled

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        |
| License       | MIT

The PR drop fixing PHP bug, that was closed in PHP 7.0

Original issues:
php/php-src#1588
symfony#16574

Original commit:
php/php-src@ff6b309

Commits
-------

03590f1 [Process] remove fixing of legacy bug, when PTS functionality is enabled
@GromNaN GromNaN force-pushed the ext-mongodb2 branch 3 times, most recently from 71f5d08 to 0a3639a Compare October 30, 2023 10:32
@GromNaN GromNaN closed this Oct 30, 2023
GromNaN pushed a commit that referenced this pull request Jan 30, 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))
symfony#4 /var/www/vendor/symfony/mailer/Mailer.php(45): Symfony\Component\Mailer\Transport\Smtp\SmtpTransport->send(Object(Symfony\Component\Mime\Email), Null)
symfony#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
GromNaN pushed a commit that referenced this pull request Mar 21, 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()
symfony#4 /vagrant/vendor/symfony/messenger/Middleware/RejectRedeliveredMessageMiddleware.php(41): Symfony\Component\Messenger\Middleware\DispatchAfterCurrentBusMiddleware->handle()
symfony#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.
GromNaN pushed a commit that referenced this pull request Sep 23, 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()
symfony#4 /tmp/symfony-yaml/vendor/symfony/yaml/Parser.php(341): Symfony\Component\Yaml\Parser->parseValue()
symfony#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
GromNaN pushed a commit that referenced this pull request Jan 4, 2025
… 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()
symfony#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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.