-
Notifications
You must be signed in to change notification settings - Fork 8k
Fix GH-20370: forbid user stream filters to violate typed property constraints #20373
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 GH-20370: forbid user stream filters to violate typed property constraints #20373
Conversation
b0808ce to
786ad32
Compare
786ad32 to
2ad8c64
Compare
ndossche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On a phone , so can't check. But you're looking at the property info now, however dynamic properties don't have any. So that means that a dynamically created property $stream would've been overwritten before this patch but now no longer. So that is an unintended behaviour change.
|
Oh alright, I wasn't aware of this subtlety of dynamic properties. Let's refine that. |
|
Hmm so that would require to someone create the dynamic property in filter first and it could then be used in subsequent calls for stream, right? If so, that sounds like a proper edge case... :) |
|
The easiest way to test this is to create a onCreate method that sets a dynamic property on $this |
2ad8c64 to
fe4961a
Compare
|
This will work, except for a pre-existing bug: It would also be great to try to avoid OBJPROP so that the property table isn't rebuilt, but that isn't critical. |
|
I tried to implement what you have in mind in 04e0955 |
7208c60 to
04e0955
Compare
04e0955 to
3d5358f
Compare
314fde5 to
c838191
Compare
arnaud-lb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ndossche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for tackling this. As I said in the issue I opened, it's a bit of an annoying issue (as you have felt).
|
Thanks @ndossche, I addressed your comments in the last commit |
ndossche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See last nit, once that's resolved I think this can be approved.
1e82ed0 to
32f5d11
Compare
|
Thanks for tackling this annoying issue |
bukka
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks correct to me. We should probably introduce some nicer api for that fake_scope as it has not been used outside zend and reflection. It should not be directly accessed filters code IMHO but new API is more master material...
ndossche
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. Go ahead.
|
Thanks for your reviews! |
Use
zend_update_property()instead of doing things "manually".