Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Messenger] Fix using negative delay#53088

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
J-roen wants to merge5 commits intosymfony:6.4fromJ-roen:fix_50833

Conversation

@J-roen
Copy link
Contributor

@J-roenJ-roen commentedDec 15, 2023
edited
Loading

QA
Branch?6.4
Bug fix?yes
New feature?no
Deprecations?no
IssuesFix#50833
LicenseMIT

Fixes using negative delays when sending messages.

@carsonbot
Copy link

Hey!

I see that this is your first PR. That is great! Welcome!

Symfony has acontribution guide which I suggest you to read.

In short:

  • Always add tests
  • Keep backward compatibility (seehttps://symfony.com/bc).
  • Bug fixes must be submitted against the lowest maintained branch where they apply (seehttps://symfony.com/releases)
  • Features and deprecations must be submitted against the 7.1 branch.

Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change.

When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor!
If this PR is merged in a lower version branch, it will be merged up to all maintained branches within a few days.

I am going to sit back now and wait for the reviews.

Cheers!

Carsonbot

@xabbuh
Copy link
Member

Actually, I do not really understand the use case for using a negative delay. Is that really something that needs to be supported?

@OskarStark
Copy link
Contributor

As far as I understand, the problem is that it does not work right now, and this can happen due to calculation, so it should "just" send the message right?

Nyholm reacted with thumbs up emoji

@valtzu
Copy link
Contributor

@xabbuh If you have a long queue, you can cut in front of the queue with this negative delay becausemessages are sorted byavailable_at.

So it's about priority within a single queue basically.

OskarStark and Nyholm reacted with thumbs up emoji

@J-roen
Copy link
ContributorAuthor

Actually, it was already possible to set negative delays, but the behaviour was unexpected (in my opinion). Another solution could be to set the time to "now" in case of a negative delay. But then use cases like described by@valtzu wouldn't be supported.

@smnandre
Copy link
Member

I'm not certain on this because:

  • we should never expect things based on bridge implementation details
  • i'm not certain it really "works" with this PR (everytime for everyone i mean).. I think about postgres notify behaviour, hard indexes on dbs... So this would need a loot of tests .. it this worth the effort ?

As i see it, you removed a valid safeguard in the code to allow a "non-written" behaviour.. and i'm pretty sure this will not lead to good things :)

And finally... there is a simpler / documented way to handle queue priority:queue priority. On a fonctional level, does this not fill your needs ?

@J-roen
Copy link
ContributorAuthor

I agree that you can discuss about the expected behaviour, but I think that what you at least wouldn't expect, is that a negative delay is converted to a positive delay. How it's supposed to work apart from that, could maybe remain undefined.

@smnandre
Copy link
Member

I agree that you can discuss about the expected behaviour, but I think that what you at least wouldn't expect, is that a negative delay is converted to a positive delay. How it's supposed to work apart from that, could maybe remain undefined.

I totally agree!

@fabpotfabpot modified the milestones:6.4,5.4Dec 19, 2023
@fabpot
Copy link
Member

@J-roen Can you create a PR for Symfony 5.4, which is also affected?

@J-roen
Copy link
ContributorAuthor

J-roen commentedDec 19, 2023
edited
Loading

@J-roen Can you create a PR for Symfony 5.4, which is also affected?

Yes, I can do that later this week. And what about 7.x?

Copy link
Member

@NyholmNyholm left a comment

Choose a reason for hiding this comment

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

Thank you

@J-roen Can you create a PR for Symfony 5.4, which is also affected?

Yes, I can do that later this week. And what about 7.x?

It is no need to add this to 7.x. When this is merged in 6.4 it will be merge "up" to 7.0 and 7.1 by a maintainer.

xabbuh reacted with thumbs up emoji
fabpot added a commit that referenced this pull requestDec 24, 2023
This PR was squashed before being merged into the 5.4 branch.Discussion----------[Messenger] Fix using negative delay| Q             | A| ------------- | ---| Branch?       | 5.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        |Fix#50833| License       | MITFixes using negative delays when sending messages. Cherry-picked from#53088.Commits-------af1517e [Messenger] Fix using negative delay
@fabpotfabpot closed thisDec 24, 2023
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@OskarStarkOskarStarkOskarStark left review comments

@fabpotfabpotfabpot approved these changes

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@NyholmNyholmNyholm approved these changes

Assignees

No one assigned

Projects

None yet

Milestone

5.4

Development

Successfully merging this pull request may close these issues.

9 participants

@J-roen@carsonbot@xabbuh@OskarStark@valtzu@smnandre@fabpot@nicolas-grekas@Nyholm

[8]ページ先頭

©2009-2025 Movatter.jp