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

Fixed backward incompatible change in php8.2 #174

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SeynovAM
Copy link

php8.2 has backward incompatible: number symbols in relative formats no longer accept multiple signs, e.g. +-2.

https://www.php.net/manual/en/migration82.incompatible.php

…ive formats no longer accept multiple signs, e.g. +-2.
$date = $this->timezoneSafeModify($date, "-{$distance} hours");
$date = $this->timezoneSafeModify($date, (-$distance) . " hours");
Copy link

Choose a reason for hiding this comment

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

Accordingly to relative formats docs this should be "hours ago"

What you wrote is just different syntax, it is being interpreted the same way - see https://3v4l.org/KRVLH

Copy link
Author

Choose a reason for hiding this comment

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

@mabar Yes, right, but it will work incorrect if $distance is negative.
For example if we are calling modify("--10 hours), we expect it like "+10 hours", but actually since php8.2 it will be working like "-10 hours": https://3v4l.org/X1XJd#v8.3.4

@dragonmantank
Copy link
Owner

This discussion and PR actually caused me to go down another path, which seemed to end up being that modify() is a bad method to be using. There is a fix that I believe fixes a number of bugs, but would like some feedback on.

Before I release a fix, could I get your thoughts on #181 as it will impact the overall solution?

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.

3 participants