Fix GH-20370: forbid user stream filters to violate typed property constraints #20373
Conversation
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
b0808ce to
786ad32
Compare
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
786ad32 to
2ad8c64
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
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 |
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
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 |
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
2 times, most recently
from
7208c60 to
04e0955
Compare
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
04e0955 to
3d5358f
Compare
alexandre-daubois
force-pushed
the
user-stream-filter-props
branch
from
314fde5 to
c838191
Compare
ndossche
left a comment
ndossche
left a comment
There was a problem hiding this comment.
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
ndossche
left a comment
There was a problem hiding this comment.
See last nit, once that's resolved I think this can be approved.
| * Since the property accepted a resource assignment above, it must have | ||
| * no type hint or be typed as mixed, so we can safely assign null. | ||
| */ | ||
| old_scope = EG(fake_scope); |
There was a problem hiding this comment.
Nit: you can move the fake scope updates inside the if-check.
5 participants
Use
zend_update_property()instead of doing things "manually".