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

Jump to bottom

Various fixes and removed deprecations #497

Merged
merged 1 commit into from
Sep 18, 2024
Merged

Various fixes and removed deprecations #497

merged 1 commit into from
Sep 18, 2024

Conversation

alexandre-daubois
Copy link
Contributor

@alexandre-daubois alexandre-daubois commented Sep 11, 2024 •
edited
Loading

First batch of fixes with the "new" CI.

@derrabus
Copy link
Member

There's one utf8_decode() occurrence left in Iconv. The recommended replacement mb_convert_encoding($string, 'ISO-8859-1', 'UTF-8') doesn't work in this case, so it deserves a bit more research. However, this fixes some problems here and there already.

We used to have a polyfill for utf8_decode() in the now-removed PHP 7.2 polyfill package. Would it help if we copied the code of that polyfill over to the iconv package?

@alexandre-daubois
Copy link
Contributor Author

Indeed @derrabus, that's smart. I copied the function and added it to Iconv as a private method. Seems to work really well 👍

stof
stof reviewed Sep 11, 2024
View reviewed changes
src/Intl/Idn/bootstrap80.php Outdated
}
if (!function_exists('idn_to_utf8')) {
function idn_to_utf8(?string $domain, ?int $flags = IDNA_DEFAULT, ?int $variant = INTL_IDNA_VARIANT_UTS46, &$idna_info = null): string|false { return p\Idn::idn_to_utf8((string) $domain, (int) $flags, (int) $variant, $idna_info); }
function idn_to_utf8(string $domain, int $flags = IDNA_DEFAULT, int $variant = INTL_IDNA_VARIANT_UTS46, &$idna_info = null): string|false { return p\Idn::idn_to_utf8($domain, $flags, $variant, $idna_info); }
Copy link
Member

Choose a reason for hiding this comment

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

Those changes should be reverted, as it makes it incompatible with passing null in non-strict-types files (which is deprecated in PHP 8.1+ but will only be removed in PHP 9.0)

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 reverted the casts

Copy link
Member

Choose a reason for hiding this comment

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

You need to revert the whole change. The most important part of my comment is about the parameter type.
PHP does not support passing null to a non-nullable userland function parameter. The reason of the PHP 8.1 deprecation is precisely because the behavior of native functions cannot be emulated exactly in userland for now (in PHP 9, they will behave the same than userland functions in that regard)

Copy link
Member

Choose a reason for hiding this comment

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

@nicolas-grekas what do you think about adding legacy tests covering passing null to all our polyfilled functions (that will have a non-nullable parameter type in PHP 9), so that the testsuite catches such broken changes to the polyfills, as that's not the first time we get such changes in a PR.

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 see what you mean, but what about this warning? This is the reason I updated the signature

Copy link
Member

Choose a reason for hiding this comment

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

@alexandre-daubois this should have been fixed by #495 AFAIK

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It doesn't seem to be touching anything related to the Incompatible signature for PHP >= 8 warning. Am I missing something?

Copy link
Member

Choose a reason for hiding this comment

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

it touches the detection logic at https://github.com/symfony/polyfill/pull/495/files#diff-05ed2f919a0a2d68912601230246f2c39077cd90c3ac9df8ac74651dd7c197aeR140

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh right I get it now. That wasn't the root cause of the warning message. That's reverted 👍

src/Iconv/Iconv.php Outdated
@@ -741,4 +741,34 @@ private static function getData($file)

return false;
}

private static function utf8Decode($s)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest naming utf8ToLatin1 instead, to reflect what this method actually does (the bad name of utf8_decode was the main reason why it has been deprecated)

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, we could drop strlen1 entirely, falling back directly to the strlen2 implementation that was used when the xml extension was not loaded and so we did not have the native utf8_decode function. The utf8_decode fallback was done because using the native function was faster than the userland logic of strlen2, but this probably does not hold anymore with a userland function doing more work.

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 went for the alternative solution, which really clean things up.

derrabus
derrabus reviewed Sep 11, 2024
View reviewed changes
tests/Php81/Php81Test.php Outdated Show resolved Hide resolved
@alexandre-daubois alexandre-daubois force-pushed the various-fixes branch 3 times, most recently from dbfb2d5 to 08a7263 Compare September 11, 2024 09:25
alexandre-daubois
alexandre-daubois commented Sep 11, 2024
View reviewed changes
tests/Php82/SensitiveParameterValuePolyfillTest.php
@@ -17,7 +17,7 @@
/**
* @author Tim Düsterhus <duesterhus@woltlab.com>
*/
class SensitiveParameterValueTest extends TestCase
class SensitiveParameterValuePolyfillTest extends TestCase
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TestListenerTrait tries to find a polyfill declaration in bootstrap.php base on the test classname. However, SensitiveParameterValue is a "special" polyfill that doesn't need any declaration in bootstrap.php.

So, PHPUnit warned that no polyfill was found, but that's normal. By renaming, the file to not match the SensitiveParametrerValue classname, we avoid TestListenerTrait to raise a wrong warning.

@alexandre-daubois alexandre-daubois force-pushed the various-fixes branch 2 times, most recently from fbd5bcb to e15b05c Compare September 11, 2024 14:16
@stof stof mentioned this pull request Sep 16, 2024
Closed
@nicolas-grekas
Copy link
Member

Thank you @alexandre-daubois.

@nicolas-grekas nicolas-grekas merged commit 0ed2048 into symfony:1.x Sep 18, 2024
0 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus derrabus left review comments

@stof stof stof left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@xabbuh xabbuh xabbuh approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alexandre-daubois @derrabus @nicolas-grekas @stof @xabbuh