Add PHP 8.0 support, require PHP 7.3 or newer#11
Add PHP 8.0 support, require PHP 7.3 or newer#11weierophinney merged 6 commits intolaminas:3.4.xfrom bfoosness:php-8.0
Conversation
| { | ||
| if (! extension_loaded('mhash')) { | ||
| $this->markTestSkipped('The mhash extension is not available'); | ||
| if (! extension_loaded('hash')) { |
There was a problem hiding this comment.
https://www.php.net/manual/en/intro.mhash.php
As of PHP 7.0.0 the Mhash extension has been fully integrated into the Hash extension. Therefore, it is no longer possible to detect Mhash support with extension_loaded(); use function_exists() instead. Furthermore, Mhash is no longer reported by get_loaded_extensions() and related features.
A check is still needed for 7.3: https://www.php.net/manual/en/hash.installation.php
As of PHP 7.4.0, the Hash extension is a core PHP extension, so it is always enabled.
Given that, I think this is the correct solution, but let me know if this should change.
There was a problem hiding this comment.
Though looking at the failed tests (Use of undefined constant MHASH_ADLER32 - assumed 'MHASH_ADLER32') in my build and #8, I suspect there's a better solution.
There was a problem hiding this comment.
Perhaps we need to check for both 'hash' && PHP_VERSION_ID >= 80000 and 'mhash' && PHP_VERSION_ID < 80000?
There was a problem hiding this comment.
The issue I'm having is that I can't reproduce this locally in either 7.3 or 7.4 using the official php docker images. This test isn't skipped for me, but I'm not seeing the same error either.
If I'm understanding the docs correctly, checking for the mhash extension is no longer possible as of PHP 7.0 (you should check for hash instead), and checking for hash is only necessary for <= PHP 7.3 because it's always enabled in later versions.
1) LaminasTest\Crypt\Key\Derivation\SaltedS2kTest::testCalc
Use of undefined constant MHASH_ADLER32 - assumed 'MHASH_ADLER32' (this will throw an Error in a future version of PHP)
It seems like this error is due strictly to the presence of the MHASH_ADLER32 constant because this test isn't even using that algo, and presumably the next constant in the list would trigger the same error.
It seems like SaltedS2k::calc should be ported to the Hash extension instead, but there's no equivalent to mhash_keygen_s2k in that extension. I think we're in a weird spot where mhash_keygen_s2k is still available, but none of its constants are. Would it be acceptable to drop support for the salted S2K algo? Or should we attempt a userland solution?
There was a problem hiding this comment.
To add to this, I believe this issue has been a problem for awhile, but this test was always being skipped:
55) LaminasTest\Crypt\Key\Derivation\SaltedS2kTest::testCalc
The mhash extension is not available
| { | ||
| public function setUp(): void | ||
| { | ||
| if (PHP_VERSION_ID >= 70100) { |
There was a problem hiding this comment.
I removed any skipped tests like this.
|
As you changed requirements with the changes in this PR, can you fill such details to PR body? |
Ocramius
left a comment
There was a problem hiding this comment.
Patch looks good, but we do indeed need to fix these before merging:
There were 4 errors:
1) LaminasTest\Crypt\Key\Derivation\SaltedS2kTest::testCalc
Use of undefined constant MHASH_ADLER32 - assumed 'MHASH_ADLER32' (this will throw an Error in a future version of PHP)
/home/travis/build/laminas/laminas-crypt/src/Key/Derivation/SaltedS2k.php:68
/home/travis/build/laminas/laminas-crypt/test/Key/Derivation/SaltedS2kTest.php:35
2) LaminasTest\Crypt\Key\Derivation\SaltedS2kTest::testCalcWithWrongHash
Use of undefined constant MHASH_ADLER32 - assumed 'MHASH_ADLER32' (this will throw an Error in a future version of PHP)
/home/travis/build/laminas/laminas-crypt/src/Key/Derivation/SaltedS2k.php:68
/home/travis/build/laminas/laminas-crypt/test/Key/Derivation/SaltedS2kTest.php:52
3) LaminasTest\Crypt\Key\Derivation\SaltedS2kTest::testCalcWithWrongSalt
Use of undefined constant MHASH_ADLER32 - assumed 'MHASH_ADLER32' (this will throw an Error in a future version of PHP)
/home/travis/build/laminas/laminas-crypt/src/Key/Derivation/SaltedS2k.php:68
/home/travis/build/laminas/laminas-crypt/test/Key/Derivation/SaltedS2kTest.php:64
4) LaminasTest\Crypt\Symmetric\OpensslAeadTest::testCcmEncryptWithTagSize
openssl_encrypt(): Setting tag length for AEAD cipher failed
/home/travis/build/laminas/laminas-crypt/src/Symmetric/Openssl.php:570
/home/travis/build/laminas/laminas-crypt/test/Symmetric/OpensslAeadTest.php:196
| { | ||
| if (! extension_loaded('mhash')) { | ||
| $this->markTestSkipped('The mhash extension is not available'); | ||
| if (! extension_loaded('hash')) { |
There was a problem hiding this comment.
Perhaps we need to check for both 'hash' && PHP_VERSION_ID >= 80000 and 'mhash' && PHP_VERSION_ID < 80000?
|
Thanks, I'll take a look today. In the meantime, laminas/laminas-math#5 is a blocker and I think it's ready to go. |
| $this->crypt->setKey(random_bytes($this->crypt->getKeySize())); | ||
| $this->crypt->setSalt(random_bytes($this->crypt->getSaltSize())); | ||
| $this->crypt->setTagSize(24); | ||
| $this->crypt->setTagSize(14); |
There was a problem hiding this comment.
This test has also been broken for awhile now, but this change fixes it.
There was a problem hiding this comment.
i saw that before and wondered how it ever worked, the tag size is max 16 for that
"In CCM, the authentication tag τ is of variable length: it is permitted to be 4, 6, 8, 10, 12, 14, or 16 bytes long."
https://csrc.nist.gov/CSRC/media/Projects/Block-Cipher-Techniques/documents/BCM/Comments/800-38-series-drafts/CCM/RW_CCM_comments.pdf
Signed-off-by: Brent Foosness <bfoosness@users.noreply.github.com>
Signed-off-by: Brent Foosness <bfoosness@users.noreply.github.com>
Signed-off-by: Brent Foosness <bfoosness@users.noreply.github.com>
… failed Signed-off-by: Brent Foosness <bfoosness@users.noreply.github.com>
Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Exclusions are no longer relevant, as they target PHP versions no longer supported with this patch. Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
|
@bfoosness I've rebased your branch off the new 3.4.x branch, which contains our GHA CI workflow, and pushed the changes back; make sure you pull! Because I rebased... I'm not sure if you've incorporated all feedback yet. Can you drop me a note and let me know? |
|
@weierophinney: Thank you! I believe #11 (review) was the only outstanding issue, and it's related to the conversation at #11 (comment) I couldn't reproduce these failures locally, and the tests now appear to pass with the GHA CI workflow. |
Description
Close #10