Unset public properties also cause calls to __unset() #1930
Conversation
|
|
||
| class Foo { | ||
| private $private; | ||
| private $protected; |
|
Hi @Ocramius -- we don't add failing tests. Are you planning on updating this to fix it? Otherwise I'll close this; it should be opened as an issue with the sample code. |
|
@scannell the PR is in place to allow merging it into a patch if somebody wants to pick it up. I will give it a shot, but I'm most probably not going to be able to fix this. If I had opened an issue, it would have still been an issue referencing a branch in a link - same as doing a PR, except that we get a nice diff view as well as reviews here :P |
|
Cool. In the meantime I'm going to close this since there's no follow-up action -- someone else can merge it into theirs if they decide to fix it. |
|
@Ocramius has a fix |
|
(CC @fredemmott) |
| @@ -0,0 +1,17 @@ | |||
| <?php | |||
There was a problem hiding this comment.
Wondering if these should be in slow or fast
There was a problem hiding this comment.
In general, everything should be in slow. The only things that go in quick are hand selected to be a litmus test that the compiler sort of works.
There was a problem hiding this comment.
but please don't just put it in the root of slow. Put it in a subdir that makes sense.
|
Is this diff waiting on something? |
|
@ptarjan except for householding (moving tests to
These are mainly string mismatches in fatals due to how the access to an invalid property is handled. |
|
@Ocramius The first 2 look completely reasonable to change the error string (what does zend output? Are we close?) The last one seems bad since it is an existing Zend test. Are you doing something wrong? Should that warning not happen in a destructor? Are you trying to do this change to be more similar to zend, and if so, why didn't they print that error? |
|
@ptarjan for the destructor one (test) it looks plain wrong - there is a scope violation, and I don't understand how it is supposed to work in Zend, but my first assumption is that the fatal is swallowed during engine teardown, while HHVM still produces output (just a speculation). In this (test) and this (test), the difference seems to lie in the fact that I deny |
|
@Ocramius if it is just error string mismatches, then just update them. The destructor is the only one I really care about. Maybe you can dig into why we still produce error and they don't? Do they set a flag that they are tearing down and don't show errors? Can you reproduce it via other means? I need you to dig more before we move the test to bad. |
|
After looking some more into it, I found out that I totally misread this test case. The fix should now be mergeable, sorry about all the fuss. (failures in travis are not related) |
|
D1211230 |
__unset()was being called only forprivateandprotectedproperties, as it seems.