Add PHP 8.0 support, require PHP 7.3 or newer #11

bfoosness · 2020-12-12T14:04:02Z

Q	A
New Feature	yes

Description

Close #10

bfoosness · 2020-12-12T14:06:11Z

test/Key/Derivation/SaltedS2kTest.php

@@ -27,8 +27,8 @@ public function setUp(): void

    public function testCalc()
    {
-        if (! extension_loaded('mhash')) {
-            $this->markTestSkipped('The mhash extension is not available');
+        if (! extension_loaded('hash')) {


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.

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.

Perhaps we need to check for both 'hash' && PHP_VERSION_ID >= 80000 and 'mhash' && PHP_VERSION_ID < 80000?

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?

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

bfoosness · 2020-12-12T14:06:44Z

test/BlockCipher/CompatibilityTest.php

-{
-    public function setUp(): void
-    {
-        if (PHP_VERSION_ID >= 70100) {


I removed any skipped tests like this.

composer.json

glensc · 2021-01-14T09:37:05Z

As you changed requirements with the changes in this PR, can you fill such details to PR body?

Ocramius

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

Ocramius · 2021-01-18T07:36:43Z

test/Key/Derivation/SaltedS2kTest.php

@@ -27,8 +27,8 @@ public function setUp(): void

    public function testCalc()
    {
-        if (! extension_loaded('mhash')) {
-            $this->markTestSkipped('The mhash extension is not available');
+        if (! extension_loaded('hash')) {


Perhaps we need to check for both 'hash' && PHP_VERSION_ID >= 80000 and 'mhash' && PHP_VERSION_ID < 80000?

bfoosness · 2021-01-18T13:24:48Z

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.

bfoosness · 2021-01-18T15:12:10Z

test/Symmetric/OpensslAeadTest.php

@@ -190,11 +190,11 @@ public function testCcmEncryptWithTagSize()
        $this->crypt->setMode('ccm');
        $this->crypt->setKey(random_bytes($this->crypt->getKeySize()));
        $this->crypt->setSalt(random_bytes($this->crypt->getSaltSize()));
-        $this->crypt->setTagSize(24);
+        $this->crypt->setTagSize(14);


This test has also been broken for awhile now, but this change fixes it.

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 <[email protected]>

… failed Signed-off-by: Brent Foosness <[email protected]>

Signed-off-by: Matthew Weier O'Phinney <[email protected]>

Exclusions are no longer relevant, as they target PHP versions no longer supported with this patch. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

weierophinney · 2021-02-11T18:28:18Z

@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?

bfoosness · 2021-02-11T18:30:59Z

@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.

bfoosness commented Dec 12, 2020

View reviewed changes

delboy1978uk mentioned this pull request Jan 13, 2021

phpunit9 and php8 laminas/laminas-mail#127

Closed

glensc reviewed Jan 14, 2021

View reviewed changes

composer.json Show resolved Hide resolved

Ocramius changed the title ~~Add PHP 8.0 support~~ Add PHP 8.0 support, require PHP 7.3 or newer Jan 18, 2021

Ocramius suggested changes Jan 18, 2021

View reviewed changes

bfoosness commented Jan 18, 2021

View reviewed changes

bfoosness added 4 commits February 11, 2021 12:11

Update composer.json and fix unit tests.

12325b1

Signed-off-by: Brent Foosness <[email protected]>

Remove tests skipped in PHP 7.3 and later.

4a34ea4

Signed-off-by: Brent Foosness <[email protected]>

Update mhash extension check.

bc1ff8e

Signed-off-by: Brent Foosness <[email protected]>

Fix test error: openssl_encrypt(): Setting tag length for AEAD cipher…

a6994cb

… failed Signed-off-by: Brent Foosness <[email protected]>

weierophinney changed the base branch from master to 3.4.x February 11, 2021 18:12

weierophinney added 2 commits February 11, 2021 12:14

fix: update composer.lock file

fec3b8e

Signed-off-by: Matthew Weier O'Phinney <[email protected]>

qa: remove CI configuration

08e03da

Exclusions are no longer relevant, as they target PHP versions no longer supported with this patch. Signed-off-by: Matthew Weier O'Phinney <[email protected]>

weierophinney added this to the 3.4.0 milestone Feb 11, 2021

weierophinney added the Enhancement label Feb 11, 2021

weierophinney merged commit a058eeb into laminas:3.4.x Feb 11, 2021

bfoosness deleted the php-8.0 branch February 11, 2021 20:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add PHP 8.0 support, require PHP 7.3 or newer #11

Add PHP 8.0 support, require PHP 7.3 or newer #11

bfoosness commented Dec 12, 2020

bfoosness Dec 12, 2020

bfoosness Dec 12, 2020 •

edited

Loading

Ocramius Jan 18, 2021

bfoosness Jan 18, 2021

bfoosness Jan 18, 2021

bfoosness Dec 12, 2020

glensc commented Jan 14, 2021

Ocramius left a comment

Ocramius Jan 18, 2021

bfoosness commented Jan 18, 2021

bfoosness Jan 18, 2021

delboy1978uk Jan 18, 2021

weierophinney commented Feb 11, 2021

bfoosness commented Feb 11, 2021

Add PHP 8.0 support, require PHP 7.3 or newer #11

Add PHP 8.0 support, require PHP 7.3 or newer #11

Conversation

bfoosness commented Dec 12, 2020

Description

Choose a reason for hiding this comment

bfoosness Dec 12, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glensc commented Jan 14, 2021

Ocramius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bfoosness commented Jan 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Feb 11, 2021

bfoosness commented Feb 11, 2021

bfoosness Dec 12, 2020 •

edited

Loading