Skip to content

[Messenger] Add AvailableAtStamp to delay messages until fixed DateTime #36512

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 3 commits into from
Closed

[Messenger] Add AvailableAtStamp to delay messages until fixed DateTime #36512

wants to merge 3 commits into from

Conversation

delolmo
Copy link

@delolmo delolmo commented Apr 21, 2020

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
License MIT

An AvailableAtStamp and an AvailableAtStampMiddleware have been added to allow posponing a message to a fixed date and time. Internally, the middleware calculates the difference in milliseconds between the desired date and the current date and adds a DelayStamp to the envelope.

@nicolas-grekas nicolas-grekas added this to the next milestone Apr 21, 2020
@delolmo
Copy link
Author

delolmo commented Apr 22, 2020

I have given more thought to this PR and I would like two things to be discussed.

  1. Calculating the amount of seconds between two dates is apparently easy, but if we are dealing with a timezone with a daylight saving time period, a +/- 1 hour difference will appear if the availableAt DateTime changes between daylight saving period and non-daylight saving period. I am no expert managing date and time functions, but could this be a solution to the problem (basically, comparing dates in UTC timezone)?
            $timezone = new DateTimeZone('UTC');

            $availableAt = new DateTime(
                $availableAtStamp
                    ->getAvailableAt()
                    ->format('Y-m-d H:i:s'),
                $timezone
            );

            $now = new DateTime(
                (new DateTime())
                    ->format('Y-m-d H:i:s'),
                $timezone
            );

            $delay = $availableAt->getTimestamp() - $now->getTimestamp();
  1. If an AvailableAtStamp is included, the default Serializer created by the Serializer Transport will now fail when trying to denormalize the DateTime object. The Symfony\Component\Serializer\Normalizer\DateTimeNormalizer should be added in the public static function create(): self method of the Serializer Transport, especially if this stamp is provided as a core function in the Messenger component. Would this be fine?

@fabpot
Copy link
Member

fabpot commented Aug 18, 2020

What's the status here?

It looks like there is an unrelated commit.

@ogizanagi
Copy link
Contributor

ogizanagi commented Aug 18, 2020

Can't we add a DelayStamp::to(\DateTime $datetime): DelayStamp helper instead of a new stamp+middleware?

@maxhelias
Copy link
Contributor

Can't we add a DelayStamp::to(\DateTime $datetime): DelayStamp helper instead of a new stamp+middleware?

@ogizanagi I agree, it could be a really good idea

@fabpot
Copy link
Member

fabpot commented Sep 5, 2020

I like @ogizanagi idea.

@weaverryan
Copy link
Member

Can't we add a DelayStamp::to(\DateTime $datetime): DelayStamp helper instead of a new stamp+middleware?

I was thinking the same! Maybe DelayStamp::delayUntil()?

@fabpot fabpot closed this Oct 7, 2020
@fabpot
Copy link
Member

fabpot commented Oct 7, 2020

We've just moved away from master as the main branch to use 5.x instead. Unfortunately, I cannot reopen the PR and change the target branch to 5.x. Can you open a new PR referencing this one to not loose the discussion? Thank you for your understanding and for your help.

@Aerendir
Copy link
Contributor

Aerendir commented Oct 8, 2020

As linked by @maxhelias , I've just proposed the exact same thing in #38459 (sorry for the duplication)...

Please, read my issue as there are some useful inspiration, I think...
Please, read also this comment, because it has some details that maybe of inspiration: #38463 (comment)

@Aerendir
Copy link
Contributor

Aerendir commented Oct 8, 2020

I've send a new PR: #38480

fabpot added a commit that referenced this pull request Oct 8, 2020
…:delayUntil() (Nyholm)

This PR was squashed before being merged into the 5.x branch.

Discussion
----------

[Messenger] Add DelayStamp::delayFor() and DelayStamp::delayUntil()

| Q             | A
| ------------- | ---
| Branch?       | 5.x
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | Fix #38141
| License       | MIT
| Doc PR        |

This PR will revert the work by @Toflar in #38141. He added some helper methods and I want to add a generic factory method that adds a DateInterval.

This PR will also close #38480 and fix #38459. It is also related to a comment in #36512 (comment) that was liked by a few people.

Maybe adding both `DelayStamp::delayFor(\DateInterval)` and `DelayStamp::delayUntil(\DateTimeInterface)` is overkill. But this covers all the bases and I hope that it will be the last change to this small class.

Commits
-------

c5de1eb [Messenger] Add DelayStamp::delayFor() and DelayStamp::delayUntil()
@nicolas-grekas nicolas-grekas modified the milestones: 5.x, 5.2 Oct 14, 2020
@delolmo delolmo deleted the available_at_stamp branch January 3, 2021 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants