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

Add a small PHP 8.0 fix #3

Merged
merged 8 commits into from
Nov 18, 2020
Merged

Add a small PHP 8.0 fix #3

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0998c41
Update Security.php
xvilo Nov 17, 2020
55716b8
Update minimum php requirement and phpunit
xvilo Nov 18, 2020
1a267d5
Update travis.yml to use php 8
xvilo Nov 18, 2020
c1b12d1
Add OS option
xvilo Nov 18, 2020
e393636
qa: update CI test matrix
weierophinney Nov 18, 2020
d0b105b
qa: use tilde version range for PHP constraint
weierophinney Nov 18, 2020
895415a
qa: update tests to make use of new PHP features
weierophinney Nov 18, 2020
ab35cfd
qa: move CS checks to PHP 7.3
weierophinney Nov 18, 2020
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
8 changes: 7 additions & 1 deletion .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,10 @@ env:
- COMPOSER_ARGS="--no-interaction"
- COVERAGE_DEPS="php-coveralls/php-coveralls"

matrix:
os:
- linux

jobs:
fast_finish: true
include:
- php: 5.6
Expand Down Expand Up @@ -44,6 +47,9 @@ matrix:
- php: 7.3
env:
- DEPS=latest
- php: 8.0
Copy link
Member

Choose a reason for hiding this comment

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

Please double check #2' checklist and compare with the current matrix.
For any version, we have DEPS=lowest and DEPS=latest.

Since 8.0 is not yet available on travis, we have to use nightly instead.
As we are dropping PHP <7.3 in composer.json, please also drop old versions from the matrix and ensure that all supported versions are being tested by travis.

This includes:

  • PHP 7.3
  • PHP 7.4
  • PHP 8.0 (nightly in travis for now)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did this initially but could test it due to travis not working. I thought at first this might have been an issue on my side. Will update this later on!

env:
- COMPOSER_ARGS="--no-interaction --ignore-platform-reqs"

before_install:
- if [[ $TEST_COVERAGE != 'true' ]]; then phpenv config-rm xdebug.ini || return 0 ; fi
Expand Down
5 changes: 3 additions & 2 deletions composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,12 +25,13 @@
}
},
"require": {
"php": "^5.6 || ^7.0",
"php": "^7.3 || ^8.0",
weierophinney marked this conversation as resolved.
Show resolved Hide resolved
"laminas/laminas-zendframework-bridge": "^1.0"
},
"require-dev": {
"laminas/laminas-coding-standard": "~1.0.0",
"phpunit/phpunit": "^5.7.27 || ^6.5.8 || ^7.1.4"
"phpunit/phpunit": "^9.4",
Copy link
Member

Choose a reason for hiding this comment

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

By bumping phpunit, you definitely have to change almost all tests contain setUp and tearDown aswell as have to check for some assertion methods.

Did you execute tests locally? You might want to fix those tests even without travis. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did run the tests on PHP 8 and via ./vendor/bin/phpunit so not sure what happened there if they would be incompatible. I will double-check and see if I did something wrong 👍

Copy link
Member

Choose a reason for hiding this comment

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

@boesing There are two test cases in the suite, and neither use setUp or tearDown, so tests actually do run!

@xvilo I'm doing updates and testing manually, and will push to your branch for final review shortly. Thanks for getting this started!

"ext-iconv": "*"
},
"autoload": {
"psr-4": {
Expand Down
12 changes: 9 additions & 3 deletions src/Security.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,9 @@ private static function scanString($xml, DOMDocument $dom = null, $libXmlConstan
}

if (! self::isPhpFpm()) {
$loadEntities = libxml_disable_entity_loader(true);
if (\PHP_VERSION_ID < 80000) {
$loadEntities = libxml_disable_entity_loader(true);
}
$useInternalXmlErrors = libxml_use_internal_errors(true);
}

Expand All @@ -75,7 +77,9 @@ private static function scanString($xml, DOMDocument $dom = null, $libXmlConstan
if (! $result) {
// Entity load to previous setting
if (! self::isPhpFpm()) {
libxml_disable_entity_loader($loadEntities);
if (\PHP_VERSION_ID < 80000) {
libxml_disable_entity_loader($loadEntities);
}
libxml_use_internal_errors($useInternalXmlErrors);
}
return false;
Expand All @@ -94,7 +98,9 @@ private static function scanString($xml, DOMDocument $dom = null, $libXmlConstan

// Entity load to previous setting
if (! self::isPhpFpm()) {
libxml_disable_entity_loader($loadEntities);
if (\PHP_VERSION_ID < 80000) {
libxml_disable_entity_loader($loadEntities);
}
libxml_use_internal_errors($useInternalXmlErrors);
}

Expand Down