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.

Sign up for GitHub

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
Draft

Remove mention of PHP 7.4 #1383

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
07ab153
Remove mention of PHP 7.4
jchaffraix Dec 6, 2024
8492274
Apply suggestions from code review
jchaffraix Dec 7, 2024
95f175f
Replace testBoolDefaultNotBool with comment
jchaffraix Dec 7, 2024
4940130
Remove str_contains shim
jchaffraix Dec 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ jobs:
strategy:
matrix:
cfg:
- { os: 'ubuntu-20.04', php: '7.4' }
- { os: 'ubuntu-22.04', php: '8.1' }
- { os: 'ubuntu-22.04', php: '8.3' }
runs-on: ${{ matrix.cfg.os }}
Expand Down Expand Up @@ -39,7 +38,6 @@ jobs:
strategy:
matrix:
cfg:
- { os: 'ubuntu-20.04', php: '7.4' }
- { os: 'ubuntu-22.04', php: '8.1' }
- { os: 'ubuntu-22.04', php: '8.3' }
runs-on: ${{ matrix.cfg.os }}
Expand Down Expand Up @@ -106,7 +104,6 @@ jobs:
strategy:
matrix:
cfg:
- { os: 'ubuntu-20.04', php: '7.4' }
- { os: 'ubuntu-22.04', php: '8.1' }
- { os: 'ubuntu-22.04', php: '8.3' }
runs-on: ${{ matrix.cfg.os }}
Expand Down
13 changes: 6 additions & 7 deletions SETUP/INSTALL.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ The following lists supported versions for the four primary middleware
components.

### PHP
PHP version 7.4 is the minimum supported version. Limited testing has been done
on PHP 8.1 and 8.3.
PHP version 8.1 is the minimum supported version. Limited testing has been done
on PHP 8.3.
jchaffraix marked this conversation as resolved.
Show resolved Hide resolved

The following PHP extensions are required. They are listed below with their
Ubuntu system package names.
Expand Down Expand Up @@ -50,11 +50,10 @@ change `core.disable_super_globals` to `false`, and flush the phpbb cache.

## Distro support
These middleware components match the following major distribution releases:
* Ubuntu 18.04, Bionic (with PHP 7.4 upgrade)
* Ubuntu 20.04, Focal
* Ubuntu 22.04, Jammy (with possible PHP 7.4 downgrade)
* RHEL / CentOS 7.x family (with PHP 7.4 and MariaDB 10.2 or later upgrade)
* RHEL / CentOS 8.x family
* Ubuntu 20.04, Focal (with PHP 8.1 upgrade)
* Ubuntu 22.04, Jammy
jchaffraix marked this conversation as resolved.
Show resolved Hide resolved
* RHEL / CentOS 8.x family (with PHP 8.1 upgrade)
* RHEL / CentOS 9.x family
Comment on lines +56 to +57
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...


## Browser support
The following are the lowest known supported browser versions for the code:
Expand Down
7 changes: 4 additions & 3 deletions SETUP/tests/unittests/ParamValidatorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -374,9 +374,10 @@ public function testBoolFalseVariants()

public function testBoolDefaultNotBool()
{
// Shockingly, PHP 7.4 (at least) 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. Maybe later versions will?
// 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.

$this->markTestSkipped('PHP will not enforce a bool type');

$this->expectException(TypeError::class);
Expand Down