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

Fixed E_* warnings generated by arrays and objects assigned to scalars #210

Closed
wants to merge 3 commits into from

Conversation

dktapps
Copy link
Contributor

@dktapps dktapps commented May 29, 2023

The included test case should explain the problem.

I don't know what the correct fix for this is right now.

Side note: Several of the tests don't work so well on PHP 8.2, generating dynamic property deprecation warnings.

The included test case should explain the problem.

I don't know what the correct fix for this is right now.
flat types can all be casted to each other, but objects and arrays can't be casted to flat types.
@dktapps
Copy link
Contributor Author

dktapps commented May 30, 2023

Proposed fix is included in the second commit.

@dktapps dktapps changed the title Added failing test case for array->string bug introduced by 67632ea5e671052b87659550fa0374883b9d77 Fixed E_* warnings generated by arrays and objects assigned to scalars May 30, 2023
dktapps added a commit to pmmp/PocketMine-MP that referenced this pull request May 30, 2023
dktapps added a commit to pmmp/PocketMine-MP that referenced this pull request May 30, 2023
@cweiske
Copy link
Owner

cweiske commented Aug 1, 2023

Before #196 (commit 576f587) , JsonMapper would throw an E_NOTICE

Array to string conversion

Since #196, a ReflectionException was thrown:

Class string does not exist

With #206 that fixes the bug introduced in #196, the old E_NOTICE behavior is back:

Array to string conversion

(Having the failing test as separate commit made this very easy to find out!)

This patch here prevents that E_NOTICE and throws a JsonMapper_Exception instead:

JSON property "pString" in class "JsonMapperTest_Object" is of type object and cannot be converted to string

This is more clear, and better than the E_NOTICE. It should also not break any existing code; at least no code that cared about E_NOTICEs.
Since code-that-does-not-care-about-E_NOTICE is affected, this will be released in a new minor version and not in a patch level version.

@cweiske cweiske closed this in ffc08ba Aug 1, 2023
@cweiske
Copy link
Owner

cweiske commented Aug 1, 2023

Thank you for the patch!

@cweiske
Copy link
Owner

cweiske commented Jan 27, 2024

Released with 4.3.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants