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

Remove mention of PHP 7.4 #1383

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jchaffraix
Copy link
Collaborator

Per the 202409 changelog, the next release will not support PHP 7.4.

This PR removes any mention of PHP 7.4. Very importantly, it removes the PHP 7.4 CI so we can rely on the updated type system in PHP 8.1 (e.g. mixed). Right now, adding those types makes PHP 7.4 CI unhappy.

Per the 202409 changelog, the next release will not support PHP 7.4.

This PR removes any mention of PHP 7.4. Very importantly, it removes
the PHP 7.4 CI so we can rely on the updated type system in PHP 8.1
(e.g. `mixed`). Right now, adding those types makes PHP 7.4 CI unhappy.
Comment on lines +55 to +56
* RHEL / CentOS 8.x family (with PHP 8.1 upgrade)
* RHEL / CentOS 9.x family
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Those are based on some Googling I did and I am not super confident about those...

Comment on lines 377 to 380
// PHP will not throw a TypeError if a non-boolean is passed
// into a function with a bool type. It instead coerces it into a bool.
// This is expected and there is nothing we can do about it...
// As it is unlikely to change, keeping this test around for documentation.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As I think this is testing a fundamental property of PHP basic types, the value of the test is very much zero and I don't think we will ever enable it. I would recommend removing it but thought I would leave this as a discussion point for reviewers in case I missed something.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with removing the function altogether but maybe we want to keep the comment somewhere in there for the next person who goes "wait, why aren't we testing the boolean one"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds good, added a comment in the test about not testing the case where $default is not a boolean.

Copy link
Member

@cpeel cpeel left a comment

Choose a reason for hiding this comment

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

Unfortunately we will likely be doing one more release on 7.4 in March before we get TEST and PROD upgraded to Ubuntu 24.04 and thus PHP 8.3. The hardware failure in October really impacted our upgrade plans 😦

SETUP/INSTALL.md Outdated Show resolved Hide resolved
SETUP/INSTALL.md Show resolved Hide resolved
@jchaffraix
Copy link
Collaborator Author

Unfortunately we will likely be doing one more release on 7.4 in March before we get TEST and PROD upgraded to Ubuntu 24.04 and thus PHP 8.3. The hardware failure in October really impacted our upgrade plans 😦

Dwang, I didn't realize we've pushed the server upgrade... 😢

Should we close this PR for now until we're ready to merge it? Or would rather we keep it (maybe as a draft) for when we're ready?

@cpeel
Copy link
Member

cpeel commented Dec 7, 2024

Should we close this PR for now until we're ready to merge it? Or would rather we keep it (maybe as a draft) for when we're ready?

Why don't we get this PR merge-ready and make it into a draft and then I can merge it in (with glee) when we're ready?

OH, and we will want to remove this section in pinc/misc.inc as part of this PR too:

if (!function_exists('str_contains')) {
    // str_contains() now exists in PHP8
    function str_contains($haystack, $needle)
    {
        return (strpos($haystack, $needle) !== false);
    }
}

@jchaffraix jchaffraix marked this pull request as draft December 7, 2024 23:04
Copy link
Member

@cpeel cpeel 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 and we should hold onto it until we have TEST (and PROD) upgraded.

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.

2 participants