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

Encoding::__toString complies with PHP specification from now on #407

Merged
merged 4 commits into from
Apr 13, 2021

Conversation

k00ni
Copy link
Collaborator

@k00ni k00ni commented Apr 6, 2021

Besides an if-else I also added more details in code and text.

Hint came from @igor-krein in #85 (comment):

Currently, in the Encoding.php, there is a "magic" function __toString() implementation, which calls a getEncodingClass() function, which, in its turn, may throw an exception (line 150).

From php.net: Warning It was not possible to throw an exception from within a __toString() method prior to PHP 7.4.0. Doing so will result in a fatal error.

I think, if there is a need to keep a PHP 7.4 support, better change the line 150 somehow (for example, simply return an empty string). Note that __toString() method must return a string.

Prior PHP 7.4 we expect an empty string to be returned (based on PHP
specification) when class is invalid.
PHP 7.4+ we expect an exception to be thrown when class is invalid.

Hint came from @igor-krein in
#85 (comment)
@k00ni k00ni added the fix label Apr 6, 2021
@k00ni k00ni requested review from j0k3r and smalot April 6, 2021 08:46
@k00ni k00ni self-assigned this Apr 6, 2021
Co-authored-by: Igor Peisakhovich <igor.krein@gmail.com>
Copy link
Collaborator

@j0k3r j0k3r left a comment

Choose a reason for hiding this comment

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

👍

@k00ni k00ni merged commit 3205823 into master Apr 13, 2021
@k00ni k00ni deleted the fix/encoding-better-tostring-handling branch April 13, 2021 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants