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

Added test for readonly properties from parent scope variable #68

Closed
wants to merge 1 commit into from

Conversation

Wedrix
Copy link

@Wedrix Wedrix commented Jul 2, 2023

This PR adds a failing test for #58.

In the test, the closure inherits an object which uses readonly properties from the parent scope.
This causes an error when the closure gets deserialised as explained by @theofidry.

@driesvints driesvints requested a review from nunomaduro July 3, 2023 07:54
@nunomaduro
Copy link
Member

Thank you for this. Are you able to now, adjust the source code to solve the issue? You probably want to add a few more tests.

@nunomaduro nunomaduro marked this pull request as draft July 3, 2023 13:48
@GrahamCampbell
Copy link
Member

This probably a no-fix. It is not possible to by-pass readonly, even using reflection.

@driesvints
Copy link
Member

@Wedrix what are your thoughts on @GrahamCampbell's comment?

@Wedrix
Copy link
Author

Wedrix commented Jul 12, 2023

@Wedrix what are your thoughts on @GrahamCampbell's comment?

Found this somewhere in the PHP docs. It may be possible depending on the implementation. I am yet to study the code.

@nunomaduro
Copy link
Member

I am afraid this issue is unfixable, because it's literally impossible to set readonly properties using reflection. Feel free to make a pull request if you can come up with a solution.

@nunomaduro nunomaduro closed this Jul 13, 2023
@Wedrix
Copy link
Author

Wedrix commented Jul 13, 2023

@nunomaduro if it cannot be fixed then maybe it should be documented as one the caveats?

@theofidry
Copy link

Thanks for investigating this @Wedrix! I remember going a bit nuts on that issue not finding the cause, good to know why now.

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.

5 participants