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

Added support for PHP 8.1 #35

Merged
merged 17 commits into from
Dec 2, 2021
Merged

Conversation

driehle
Copy link
Contributor

@driehle driehle commented Nov 28, 2021

Q A
Documentation no
Bugfix no
BC Break no
New Feature yes
RFC yes
QA no

Description

This PR adds support for PHP 8.1. Besides, laminas-coding-standard has been updated to 2.0.

Note that I have suppressed a couple of type hint errors in phpcs.xml. Those rules are simply too noisy and I cannot fix all the issues there.

@driehle driehle marked this pull request as ready for review November 28, 2021 18:51
@driehle driehle mentioned this pull request Nov 28, 2021
@tobias-trozowski tobias-trozowski mentioned this pull request Nov 28, 2021
phpcs.xml Show resolved Hide resolved
src/Compress/Gz.php Show resolved Hide resolved
src/DenyList.php Outdated Show resolved Hide resolved
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
…s in gz decompress

Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
… in PHP 7.3 and 7.4

Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
src/FilterPluginManager.php Outdated Show resolved Hide resolved
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
]]
];
foreach ($this->returnFilterType() as $parameter) {
yield [$parameter[0], new stdClass()];
Copy link
Contributor

Choose a reason for hiding this comment

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

previously available null was missing here

Suggested change
yield [$parameter[0], new stdClass()];
yield [null];
yield [$parameter[0], new stdClass()];

Copy link
Contributor Author

@driehle driehle Nov 29, 2021

Choose a reason for hiding this comment

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

It went further down into testDecompressNullValueThrowsRuntimeException. As that one is supposed to throw an exception, handling it in the test case above doesn't make sense to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

in the previous version the test was expecting null as return when null was provided as argument, so this PR is actually introducing a new behaviour.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I see. It looks like bzdecompress(null) actually works:

driehle:/laminas-filter$ php7.3 -r "var_dump(bzdecompress(null));"
string(0) ""
driehle:/laminas-filter$ php7.4 -r "var_dump(bzdecompress(null));"
string(0) ""
driehle:/laminas-filter$ php8.0 -r "var_dump(bzdecompress(null));"
string(0) ""
driehle:/laminas-filter$ php8.1 -r "var_dump(bzdecompress(null));"
string(0) ""

That is different from gzuncompress(null), which has never worked and which throws a type incompatibility error with PHP 8.1:

https://3v4l.org/VJfrk

Deprecated: gzuncompress(): Passing null to parameter #1 ($data) of type string is deprecated in /in/VJfrk on line 3

Warning: gzuncompress(): data error in /in/VJfrk on line 3
bool(false)

Fatal error: Uncaught Error: Call to undefined function bzdecompress() in /in/VJfrk:6
Stack trace:
#0 {main}
  thrown in /in/VJfrk on line 6

So we would have to keep accepting decompress(null) for the Bz2 filter, but allow it with the Gz filter... :-\

Copy link
Contributor

Choose a reason for hiding this comment

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

i would love to see all this fully typed and non-null... but yeah... need to keep accepting this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, there is an issue. Updating to laminas-coding-standard 2.0 has introduced declare(strict_types=1) in all files. This doesn't allow passing null to bzuncompress() anymore withouth getting a TypeError. I didn't recognize this at first, as my console commands posted above didn't include strict types. Here is the updated output:

driehle:/laminas-filter$ php7.3 -r "declare(strict_types=1);bzdecompress(null);"
PHP Fatal error:  Uncaught TypeError: bzdecompress() expects parameter 1 to be string, null given in Command line code:1
Stack trace:
#0 Command line code(1): bzdecompress(NULL)
#1 {main}
  thrown in Command line code on line 1

Output is the same for PHP 7.3, 7.4, 8.0 and 8.1.

I'll add something to cover null explicitly in the code.

Copy link
Contributor

Choose a reason for hiding this comment

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

the declare strict only kicks in for files which define it. if a have a test.php w/o declare(strict_types=1) there wont be a type error.

https://www.php.net/manual/en/language.types.declarations.php#language.types.declarations.strict

Strict typing applies to function calls made from within the file with strict typing enabled, not to the functions declared within that file. If a file without strict typing enabled makes a call to a function that was defined in a file with strict typing, the caller's preference (coercive typing) will be respected, and the value will be coerced.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, but strict types are declared in our tests as well. I still think that we should cover this in the source files, so that it works for both users having strict types enabled or not enabled. See 289743d for a proposed solution.

Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
@froschdesign froschdesign linked an issue Nov 29, 2021 that may be closed by this pull request
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
@driehle driehle force-pushed the feature/php81 branch 2 times, most recently from 59774d4 to 1c7dc53 Compare November 29, 2021 10:00
Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
$filter = new GzCompression();
$result = $filter->decompress(null);

$this->assertEmpty($result);
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't be needed as decompress method is throwing exception with null arg

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed with b9e3ef4

Signed-off-by: Dennis Riehle <webmaster@riehle-web.com>
@acelaya acelaya mentioned this pull request Dec 2, 2021
25 tasks
@driehle
Copy link
Contributor Author

driehle commented Dec 2, 2021

Is there something I can further help with?

Helps us identify deprecated behavior early so we can make code forwards compatible.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
7.3 hits EOL in less than a week; no reason to support it in a new version.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Due to the definition of `FilterPluginManagerFactory::__invoke()`, and how it then calls the `FilterPluginManager` constructor.
No checks are needed at this time.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
Fixes deprecation notice.

Signed-off-by: Matthew Weier O'Phinney <matthew@weierophinney.net>
@weierophinney weierophinney added this to the 2.13.0 milestone Dec 2, 2021
@weierophinney weierophinney merged commit 67d88c4 into laminas:2.13.x Dec 2, 2021
@driehle driehle deleted the feature/php81 branch December 2, 2021 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 8.1 support
4 participants