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

Fix PHP 8.1 compatibility issues #374

Merged
merged 6 commits into from
Oct 23, 2021
Merged

Conversation

ediamin
Copy link
Collaborator

@ediamin ediamin commented Oct 4, 2021

The goal of this PR is to fix some error and warning notices when run in the PHP 8.1.

Fixes #362

@ediamin
Copy link
Collaborator Author

ediamin commented Oct 4, 2021

@schlessera I've followed the suggestion by swissspidy. But still having some tests failure related to the encoding in DocumentTest. So far I've found out that in PHP 8.1 mb_detect_encoding works differently. Please see this line in DocumentEncoding class. For the failed tests the $detectedEncoding is ISO-8859-15 except for PHP 8.1. In PHP 8.1 its value is JIS. I tried to put the ISO-8859-15 before JIS in Encoding::DETECTION_ORDER., but this new order causes problems in other tests.

Please suggest how should I proceed.

@schlessera
Copy link
Collaborator

Related upstream issue: https://bugs.php.net/bug.php?id=81390

It seems like the algorithm for detecting the encoding has changed in PHP 8.1. The best approach right now seems to drastically limit the list of available encodings to more closely match was PHP 8.0 and lower was doing.

In the above issue, something like this is suggested:

$encodings = ['UTF-8', 'SJIS', 'GB2312',
         'ISO-8859-1', 'ISO-8859-2', 'ISO-8859-3', 'ISO-8859-4',
         'ISO-8859-5', 'ISO-8859-6', 'ISO-8859-7', 'ISO-8859-8', 'ISO-8859-9',
         'ISO-8859-10', 'ISO-8859-13', 'ISO-8859-14', 'ISO-8859-15', 'ISO-8859-16',
         'WINDOWS-1252', 'WINDOWS-1251', 'EUC-JP', 'EUC-TW', 'KOI8-R', 'BIG-5',
         'ISO-2022-KR', 'ISO-2022-JP', 'UTF-16'
];

However, this encoding order might cause regressions in PHP 8.0 and lower, so we'll have to check for that and maybe even split the encoding order here by PHP version.

Comment on lines +75 to +76
// Make sure $value is always a string and not null.
$value = $value ? $value : '';
Copy link
Contributor

Choose a reason for hiding this comment

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

If someone passes null, shouldn't PHP yell at them for it? Converting null to empty string here would hide any potential issues in consumer code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've tested this scenario with following code snippet and found out that if we pass null as the function argument, then $value will be Null not an empty string,

<?php

function test($a, $b = '')
{
    var_dump($a);
    var_dump($b); // $b is Null for second function call.
}

test('without b');
test('with null', null);

@ediamin
Copy link
Collaborator Author

ediamin commented Oct 5, 2021

@schlessera I've updated the existing encoding order. Looks like I did some mistake when I tried to change that order before. But it is working now for all tests. FYI the suggested encoding array does not work. PHP 8.1 produces different result than its lower versions.

@ediamin ediamin marked this pull request as ready for review October 5, 2021 12:05
@schlessera
Copy link
Collaborator

As discussed during our meeting, this needs many more test cases covering as many different encodings as possible, to ensure we consistently have the same behavior between PHP 8.1 and PHP 8.0 and lower.

@schlessera
Copy link
Collaborator

@ediamin Note that the files in the src/Validator folder are being auto-generated. You'll have to adapt the source templates, not the generated files.

@ediamin
Copy link
Collaborator Author

ediamin commented Oct 12, 2021

@schlessera I've updated the code with some new test cases. As you suggested earlier, I had to use a separate order list for PHP 8.1.

@ediamin ediamin force-pushed the fix/362-php-8-1-compatibility branch from da85c47 to c802630 Compare October 18, 2021 11:54
@ediamin
Copy link
Collaborator Author

ediamin commented Oct 18, 2021

@schlessera I've updated the code and added the ReturnTypeWillChange in validator source files.

@schlessera schlessera added the Tech Debt Deprecations, inefficiencies, code health label Oct 23, 2021
@schlessera schlessera added this to the 0.8.0 milestone Oct 23, 2021
@schlessera schlessera merged commit 6197885 into main Oct 23, 2021
@schlessera schlessera deleted the fix/362-php-8-1-compatibility branch October 23, 2021 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tech Debt Deprecations, inefficiencies, code health
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PHP 8.1 Compatibility
3 participants