Skip to content
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

Rework the messenger integration #7253

Merged
merged 21 commits into from
Jun 6, 2024

Conversation

Toflar
Copy link
Member

@Toflar Toflar commented May 30, 2024

Fixes #7251

  • All the Messenger magic is gone, we now always use the Doctrine queue and all messages are always sent to the queue.
  • Everything that used the SyncTransport before is now automatically delayed to the kernel.terminate event providing an advantage for all users (that have access to fastcgi_finish_request()), not only the ones that have the cronjob configured.
  • Logic spread across 4 classes is now centralized in 1 🥳
  • It fixes the You cannot call ack() on the Messenger SyncTransport. no matter the grace period because the transport configuration does not change anymore.
  • The grace period has been adjusted to 10 minutes and is now also configurable

I've split deleting the old logic and the new in two commits so that it's easier to review.

Remarks:

  • I decided to not create separate *Listener classes because they would all be one-liners forwarding to the WebWorker service. Now, all the magic is part of the WebWorker which makes it way easier to understand and see what's going on. I know we have *Listener classes for almost everything else but I strongly suggest to not do that in this case. It also makes removing the one single service from the container super easy in case no transport is configured to use the web worker (core-bundle standalone default).

@Toflar Toflar self-assigned this May 30, 2024
@Toflar Toflar added this to the 5.3 milestone May 30, 2024
@Toflar Toflar added the bug label May 30, 2024
@Toflar Toflar force-pushed the fix/messenger-fallback-logic branch from c264ab0 to 4749d24 Compare May 31, 2024 07:02
@Toflar Toflar force-pushed the fix/messenger-fallback-logic branch from 4749d24 to b9af073 Compare May 31, 2024 12:59
@Toflar
Copy link
Member Author

Toflar commented Jun 3, 2024

@ausi this is almost ready for review. There's a problem with your GlobalStateWatcher though, it detects that [Symfony\Component\Clock\Clock::globalClock] is set during the tests which is correct but there's no way to fix this. It's instantiated in the Worker and there's no way to pass a mock object there. Also, there's no way to reset the Clock to null once it's set. I think this very special case should be ignored as the Clock class is designed to work like this. Wdyt?

@ausi
Copy link
Member

ausi commented Jun 3, 2024

I think this very special case should be ignored as the Clock class is designed to work like this. Wdyt?

Yes, it should be added to the ignore list here:

'Symfony\Component\Config\Resource\ComposerResource',

@ausi
Copy link
Member

ausi commented Jun 3, 2024

Is there an easy way to remove the info output in the tests?
Bildschirmfoto 2024-06-03 um 17 47 00

@Toflar Toflar removed the unconfirmed label Jun 4, 2024
@Toflar Toflar marked this pull request as ready for review June 4, 2024 07:07
@Toflar Toflar requested review from ausi and fritzmg June 4, 2024 07:07
ausi
ausi previously approved these changes Jun 4, 2024
Copy link
Member

@ausi ausi left a comment

Choose a reason for hiding this comment

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

LGTM 🎉
(Did not test it manually)

Co-authored-by: Leo Feyer <1192057+leofeyer@users.noreply.github.com>
@leofeyer leofeyer changed the title Reworked Messenger integration Rework the messenger integration Jun 6, 2024
@leofeyer leofeyer merged commit 2b4ccc3 into contao:5.3 Jun 6, 2024
18 checks passed
@leofeyer
Copy link
Member

leofeyer commented Jun 6, 2024

Thank you @Toflar.

@Toflar Toflar deleted the fix/messenger-fallback-logic branch June 27, 2024 08:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants