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

Suppress E_WARNING #1091

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Suppress E_WARNING #1091

wants to merge 4 commits into from

Conversation

ulrichmathes
Copy link

@ulrichmathes ulrichmathes commented Aug 7, 2024 •
edited
Loading

  • [BUGFIX] don't convert empty string
  • [BUGFIX] suppress unserialize E_WARNINGS

Description

Solves #1087

Prevents E_WANRINGS in JsonCompatibilityConverter
by early return on empty strings and fixing
the error handling of unserialize

I have

  • Checked that CGL are followed
  • Checked that the Tests are still working [unit, functional are not running in devbox?]
  • Added description to CHANGELOG.md (github-handle is optional)
  • Added tests for the new code

ulrichmathes and others added 4 commits August 7, 2024 07:51
@ulrichmathes
[BUGFIX] don't convert empty string
4d508de 
Fixes tomasnorre#1087

Return early on empty $dataString as this can't be
an array in any case.
@ulrichmathes
[BUGFIX] suppress unserialize E_WARNINGS
d7fcfb3 
Fixes tomasnorre#1087

The intention of the try-catch block was to
catch Throwables during the unserialization of $dataString.

However, unserialize only throws exceptions when the
initialization of objects fails. With 'allowed_classes' => false,
no objects will be initialized.

Since PHP 8.3, an E_WARNING is issued if the string
is not unserializable. As we are not certain
if the string contains serialized data, we only want
to attempt to unserialize it.

To prevent E_WARNINGS, we need to suppress errors
instead of using try-catch.
@ulrichmathes
[TASK] add Changelog for issue tomasnorre#1087 pr
457754b
@tomasnorre
Merge branch 'main' into bugfix/1087
f36836a
@tomasnorre tomasnorre mentioned this pull request Oct 6, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ulrichmathes @tomasnorre