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

Support 32-bit PHP versions in event listing #814

Closed
wants to merge 3 commits into from
Closed

Conversation

m-vo
Copy link
Member

@m-vo m-vo commented Oct 2, 2019

see #811

limit 'next_all' to PHP_INT_MAX
@leofeyer leofeyer added the bug label Oct 2, 2019
@leofeyer leofeyer added this to the 4.8 milestone Oct 2, 2019
@aschempp
Copy link
Member

warum verwenden wir nicht direkt PHP_INT_MAX?

@leofeyer
Copy link
Member

Weil das auf 64bit-Systemen viel zu groß ist und zu einem Memory-Problem bei unendlichen wiederholten Events führen würde. (Wir hatten es zuerst mit PHP_INT_MAX versucht.)

@leofeyer
Copy link
Member

Es gibt noch einige andere Stellen, an denen wir 4294967295 verwenden. @m-vo Müssen wir die nicht auch anpassen?

@m-vo
Copy link
Member Author

m-vo commented Oct 18, 2019

Stimmt, du hast natürlich recht.

I was about to apply the changes everywhere, but honestly I think we should just set the value to 2147483647. That's PHP_INT_MAX on 32bit systems. This translates to 2038-01-19 05:14:07 (vs. 2106-02-07 07:28:15).

This way databases created under 64bit systems are interchangeable with 32bit ones. Alternatively we could change the values to floats everywhere, but than we would probably also need to change method signatures (BC...) and introduce another class of problems (precision).

@leofeyer
Copy link
Member

The whole point of #510 was not to use 2147483647 anymore, so going back there is not an option.

@m-vo
Copy link
Member Author

m-vo commented Oct 19, 2019

Right. I'll see if there is another approach.
(Like maybe a special value that means 'max' in the database and a platform dependent interpretation when listing.)

@m-vo
Copy link
Member Author

m-vo commented Oct 28, 2019

@leofeyer Can we safely change the annotated types in the legacy code (see example below) without breaking BC? Integers passed in by 3rd party code would coerce to floats - we wouldn't be able to add type hints in the method signature, though.

/*
* @param integer $value
*/
function someFunction($value) 
{
    // ...
}

/*
* @param float $value
*/
function someFunction($value) 
{
    // ...
}

This way we would not need to differentiate between platforms.

@m-vo
Copy link
Member Author

m-vo commented Oct 28, 2019

This is a tricky one.

For a simple fix it would be enough to allow the model's repository functions to work with floats as well. In fact there is only one location where this fails, namely here (due to the explicit cast):

$intStart = (int) $intStart;
$intEnd = (int) $intEnd;

For now I changed the models method signatures to also allow floats. With this, however, we silently ignore that 32 bit PHP will apply type juggling in the code before calling these functions ($intStart in getAllEvents() will then be a float for example).

I don't see how this could be easily changed though. There is even a hook where the values are passed around. If implementers type hint against an integer there, the code will break. 🤷‍♂️

@leofeyer
Copy link
Member

leofeyer commented Nov 4, 2019

See #918.

@leofeyer leofeyer closed this Nov 4, 2019
@m-vo m-vo deleted the patch-2 branch January 2, 2021 12:06
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.

3 participants