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 issue with dynamic array fields in PHP 8.2 #44

Closed
wants to merge 1 commit into from
Closed

Fix issue with dynamic array fields in PHP 8.2 #44

wants to merge 1 commit into from

Conversation

mdio
Copy link

@mdio mdio commented Dec 18, 2023

Would issue "Indirect modification of overloaded property has no effect" in Unmarshaller.php:353 and not set the value otherwise.

The problem is that

$this->{$field}[$k] = $this->buildScalarValue($field, $v, $type, false);

will first call AbstractModel::__get($field) and then AbstractModel::__set($field) with [$k] modified and not AbstractModel::__set($field[$k]) immediately.
Because __get(() didn't return a reference, the __set() didn't have an actual effect.

I went with return the value from __get() via reference, because it needed only a few lines of changes.

Another option would have been to build up the array (call it e.g. $unmarshalledArray) for $field in unmarshalArray() and issue one $this->{$field} = $unmarshalledArray;. This would bypass __get() and immediately call __set(), avoiding the issue as well.

Would issue "Indirect modification of overloaded property has no effect" and not set the value otherwise.
@mdio mdio requested a review from dcarbone as a code owner December 18, 2023 10:47
@dcarbone
Copy link
Owner

@mdio sorry its taken a little while, but I've released v2.0.0 that drops support for php 7, and includes a fix for this issue.

Please try that release and let me know if that works for you.

@mdio
Copy link
Author

mdio commented Feb 26, 2024

Hey @dcarbone, sorry for the delay. I was able to update to v2.0.0 today and can confirm that it works and doesn't have any deprecation warnings while running on PHP 8.2.
Thank you!

Since the problem has been fixed, I'm closing this PR.

@mdio mdio closed this Feb 26, 2024
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.

2 participants