Skip to content

Conversation

jrushlow
Copy link
Contributor

@jrushlow jrushlow commented Aug 6, 2022

Waiting For:

  • PHP 8.1.10 is released.

php/php-src#8730 has been merged in the php/php-src which fixes the diff() bug in PHP 8.1.

Test failed in PHP 8.1 - 8.1.9 ->https://3v4l.org/AZpkv

Test now passes against master -> https://3v4l.org/AZpkv/rfc

Once PHP 8.1.10 is released, this PR will prevent users from experiencing this bug in 8.1 through 8.1.9

fixes #219

@ovgray
Copy link

ovgray commented Aug 18, 2022

It seems to me that this "fix" will simply prevent those stuck on php >=8.1 <8.1.10 from continuing to use this bundle. Not everyone can update their php version the moment a new one is released.

Surely it would be better to adopt a fix that will work on php >=8.1 <8.1.10.

@jrushlow jrushlow changed the title WIP - fix expiration diff bug in php 8.1 fix expiration diff bug in php 8.1 Sep 2, 2022
@jrushlow jrushlow removed the Status: Needs Review Needs to be reviewed label Sep 2, 2022
@jrushlow jrushlow merged commit 1cb8446 into SymfonyCasts:main Sep 2, 2022
@jrushlow jrushlow deleted the fix/diff-bug branch September 2, 2022 19:38
@jrushlow
Copy link
Contributor Author

jrushlow commented Sep 2, 2022

It seems to me that this "fix" will simply prevent those stuck on php >=8.1 <8.1.10 from continuing to use this bundle. Not everyone can update their php version the moment a new one is released.

Surely it would be better to adopt a fix that will work on php >=8.1 <8.1.10.

Because the bug exists outside of this bundle, a fix for a fix doesn't make much sense here. Granted I understand where you're coming from - but to implement a fix for this bug to satisfy version constraints between 8.1.0 -> 8.1.9 would be a BC break here. The work around would be to support multiple major versions of this bundle, which is a lot of technical debt && an even bigger maintenance burden.

@ovgray
Copy link

ovgray commented Sep 4, 2022

@jrushlow
Just to be clear, I was not advocating having different code for php >=8.1 <8.1.1.

My thought was you could change your code as proposed by wvandenhaak at [(https://github.com//issues/219)] to avoid the bug in php >=8.1 <8.1 altogether. I cannot see why that would that would not work in all supported versions of php, or create any greater maintenance burden than the existing code.

But its your project, for which I and others are grateful, and if you are not inclined to provide more inclusive code that is your prerogative.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Bug Fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect expiration time rendering with PHP 8.1 bug

2 participants