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

Fix #70153 \DateInterval incorrectly unserialized #4687

Closed
wants to merge 5 commits into from
Closed

Fix #70153 \DateInterval incorrectly unserialized #4687

wants to merge 5 commits into from

Conversation

iakunin
Copy link

@iakunin iakunin commented Sep 6, 2019

Fixes: https://bugs.php.net/bug.php?id=70153

Added a special handling for the days property, so that bool(false) is
correctly converted to the proper internal representation.

Solution is based on Christoph M. Becker's (@cmb69) patch
proposed in the bug page.

Added a special handling for the days property, so that bool(false) is
correctly converted to the proper internal representation.

Solution is based on Christoph M. Becker's (https://people.php.net/cmb) patch
proposed in the bug page.
Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thanks! I've completely forgotten this one.

@derickr Could you please check?

ext/date/php_date.c Outdated Show resolved Hide resolved
Using the proper accessor macro

Co-Authored-By: Christoph M. Becker <[email protected]>
@iakunin iakunin requested a review from cmb69 September 6, 2019 16:34
DATE_A64I((*intobj)->diff->member, ZSTR_VAL(str)); \
} else { \
(*intobj)->diff->member = -99999; \
} \
Copy link
Member

Choose a reason for hiding this comment

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

I think it would make more sense to treat days separately, rather than trying to fit it into this macro (and thus giving all other properties the same behavior, even though it doesn't make sense there).

We could then also explicitly check for a false value, rather than "" === (string)false, as what is done now.

Copy link
Author

Choose a reason for hiding this comment

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

I've created a separate macro for handling days property.

But I'm not sure if it's the best way to solve this problem.

Added a separate macro for reading 'days' property.
PHP_DATE_INTERVAL_READ_PROPERTY_I64 reverted to its original state.
@iakunin
Copy link
Author

iakunin commented Sep 9, 2019

By the way, I have no idea why this build failed. I just started it one more time and it succeeded.

There was some error with PostgreSQL pg_fetch_*() functions (ext/pgsql/tests/17result.phpt).

@nikic, @cmb69, is it worth worrying about?

@cmb69
Copy link
Member

cmb69 commented Sep 9, 2019

I assume that the test failure is intermittent, and not related to this PR.

@iakunin
Copy link
Author

iakunin commented Sep 14, 2019

@nikic, @cmb69, hey guys!

Sorry for being so impatient, but how long does it usually takes for PR to be merged?

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

Thanks! LGTM, but I think @derickr should have a look.

ext/date/php_date.c Show resolved Hide resolved
ext/date/php_date.c Outdated Show resolved Hide resolved
@iakunin
Copy link
Author

iakunin commented Sep 23, 2019

@derickr, @cmb69, could you review it one more time, please?

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

This looks good to me. Thanks.

@iakunin
Copy link
Author

iakunin commented Sep 25, 2019

@derickr could you review it one more time, please?

@iakunin
Copy link
Author

iakunin commented Oct 18, 2019

@derickr could you review it one more time, please?

@derickr
Copy link
Member

derickr commented Oct 18, 2019

LGTM

@iakunin
Copy link
Author

iakunin commented Oct 18, 2019

@derickr thank you so much!

@cmb69
Copy link
Member

cmb69 commented Oct 18, 2019

Thanks! Applied as d2cde0b.

@cmb69 cmb69 closed this Oct 18, 2019
php-pulls pushed a commit that referenced this pull request Oct 18, 2019
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.

7 participants