-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Messenger] Doctrine setup with migrations #40336
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
Conversation
About #39928. The issue is when we run If, however, some metadata was retuned, then doctrine would trigger event I also cannot reproduce this comment by @OskarStark #39928 (comment). I might be missing something, but I don't think #40055 is an actual fix for #39928. |
👎 When using a migration to setup the PotgreSQL transport, we are already in a transaction, so adding another one breaks. Reverting my PR would re-introduce the bug. |
After some more investigating and help from @derrabus. I've managed to reproduce the issue in #39928. On PHP 7.4 everything is working as expected. But on PHP 8, we need #40055 because it fixes the issue with nested transactions. Without #40055 we get an exception like
When running migrations in a transaction (default). Then:
Why this don't happen in PHP7 is beyond me. @derrabus says that the PDO has become a bit more aware on the transaction state in PHP 8.0. The solution I propose is to try to start the transaction but never run
We don't need FYI Since the SQL includes a |
Do we agree that we can't setup the transport with migrations when we don't have any entity? It's an edge case but maybe this tell us that setting up transports using migrations is not the right solution. What about using |
Thank you for the suggestion. But wouldn’t that trigger an event at every request? If you don’t have any entities, why would you use migrations? The transport can create the table in prod when you first need the table. |
So let's give up on this edge case. My suggestion was to use This code is only called by We just have to be careful with |
Not directly related to this PR, but the more I think about it, the more I think we should remove the |
I think it make sense to remove all "auto-setup" features for all transports. They are just making all transports filled with options and complexity. It is a different discussion, I'll start a new issue. |
I agree, its to much magic which can cause weird errors and does not feel very stable. |
I opened the issue: #40361 |
Thank you @Nyholm. |
This PR was merged into the 5.2 branch. Discussion ---------- [Messenger] Don't lock tables or start transactions | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT | Doc PR | I was so sure my PR #40336 fixed the doctrine-messenger issue once and for all. But it had a very silent bug.. This has been tricky to debug and find because the `PDO` behaves differently in PHP 7.4 compared to PHP 8. | Scenario | Command | Is executed in transaction | Method that calls `PostgreSqlConnection::getTriggerSql()` | | --| -- | -- | -- | | A | `messenger:setup-transports` | No | `setup()` | B| `doctrine:schema:create` | No | `getExtraSetupSqlForTable()` | C | `doctrine:migration:diff` | Yes by default, but it can be configured | `getExtraSetupSqlForTable()` PR #40055 fixed scenario C on PHP 8, but that also broke scenario B on PHP 7.4 and PHP 8. In PR #40336 I was wrong claiming: > We don't need COMMIT because the transaction will commit itself when we close the connection. The result was the we removed all the errors messages from the 3 scenarios. But scenario B will produce some SQL that is actually never committed. IE it will silently fail. I've been trying to figure out a good solution to how or when to start a transaction. I tried out @fbourigault [suggestion](#40336 (comment)) but that would be the same fix as #40055. --------------- We need a transaction because the SQL includes a `LOCK TABLE`, however, I cannot see a strict need for it. This PR removes `LOCK TABLE` and all transaction juggling. It all seams to work. I would be happy to get thorough feedback on this PR so we can end the chapter of constantly adding bugs to this part of the component. @dunglas, you added `LOCK TABLE` in your initial version of this class in #35485, could you share some knowledge if this is a good or bad idea? Commits ------- 26061a1 Dont lock tables or start transactions
This PR reverts parts of #40055.
When running these commands, You do need to be in a transaction:
doctrine:schema:create
messenger:setup-transports
doctrine:migrations:diff
anddoctrine:migrations:migrate