Skip to content

Fix #70153 \DateInterval incorrectly unserialized#4687

Closed
iakunin wants to merge 5 commits into
php:PHP-7.2from
iakunin:fix-70153
Closed

Fix #70153 \DateInterval incorrectly unserialized#4687
iakunin wants to merge 5 commits into
php:PHP-7.2from
iakunin:fix-70153

Conversation

@iakunin

@iakunin iakunin commented Sep 6, 2019

Copy link
Copy Markdown

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.

@cmb69 cmb69 left a comment

Copy link
Copy Markdown
Member

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?

Comment thread ext/date/php_date.c Outdated
Using the proper accessor macro

Co-Authored-By: Christoph M. Becker <cmbecker69@gmx.de>
@iakunin iakunin requested a review from cmb69 September 6, 2019 16:34
Comment thread ext/date/php_date.c Outdated
DATE_A64I((*intobj)->diff->member, ZSTR_VAL(str)); \
} else { \
(*intobj)->diff->member = -99999; \
} \

Copy link
Copy Markdown
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
Copy Markdown
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 iakunin requested a review from nikic September 8, 2019 07:53
@iakunin

iakunin commented Sep 9, 2019

Copy link
Copy Markdown
Author

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

cmb69 commented Sep 9, 2019

Copy link
Copy Markdown
Member

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

@iakunin

iakunin commented Sep 14, 2019

Copy link
Copy Markdown
Author

@nikic, @cmb69, hey guys!

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

@cmb69 cmb69 left a comment

Copy link
Copy Markdown
Member

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.

Comment thread ext/date/php_date.c
Comment thread ext/date/php_date.c Outdated
@iakunin iakunin requested review from cmb69 and derickr September 17, 2019 18:27
@iakunin

iakunin commented Sep 23, 2019

Copy link
Copy Markdown
Author

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

@cmb69 cmb69 left a comment

Copy link
Copy Markdown
Member

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

iakunin commented Sep 25, 2019

Copy link
Copy Markdown
Author

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

@iakunin

iakunin commented Oct 18, 2019

Copy link
Copy Markdown
Author

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

@derickr

derickr commented Oct 18, 2019

Copy link
Copy Markdown
Member

LGTM

@iakunin

iakunin commented Oct 18, 2019

Copy link
Copy Markdown
Author

@derickr thank you so much!

@cmb69

cmb69 commented Oct 18, 2019

Copy link
Copy Markdown
Member

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