Tests for additional "standard property use cases" #18

Closed
cpriest opened this Issue Dec 29, 2012 · 12 comments

Projects

None yet

3 participants

@cpriest
Owner
cpriest commented Dec 29, 2012
  1. $foo->bar[] = 1
  2. $foo->bar[123] = 4
  3. $foobar = &$foo->bar
  4. $foobar->baz = $x with $bar being object or empty or non-object value (proper error and no leaks – Stas)
  5. $foo->bar{$x} = “foo” (string offsets), also checks that non-string vars work fine
  6. $foo->bar = &bar->baz, etc. with modifying both sides and checking everything works (including modifying via accessors – Stas)
Owner
cpriest commented Dec 29, 2012

There isn't perfect clarity on what stas was meaning by #6 in the above list. Perhaps @smalyshev can expand on what he meant.

I took it to mean that @foo->bar was a regular property and &bar->baz was an accessor which did not return a reference, in which case we would need an accessor proxy object.

What I meant is that the test should ensure this works:

$foo->bar = &$bar->baz;
$foo->bar  = 42;
echo $bar->baz;

and this works:

$foo->bar = &$bar->baz;
$bar->baz  = 42;
echo $foo->bar;

and both produce 42, and that includes the case where either of $foo->bar or $baz->baz or both of them are accessed via the use of accessors.

Owner
cpriest commented Dec 29, 2012

And are you speaking strictly of the case where the accessors return
references?

On 12/29/2012 3:42 PM, Stanislav Malyshev wrote:

What I meant is that the test should ensure this works:

$foo->bar = &$bar->baz;
$foo->bar = 42;
echo $bar->baz;

and this works:

$foo->bar = &$bar->baz;
$bar->baz = 42;
echo $foo->bar;

and both produce 42, and that includes the case where either of
$foo->bar or $baz->baz or both of them are accessed via the use of
accessors.


Reply to this email directly or view it on GitHub
#18 (comment).

I'm not sure I understand what are you asking. Accessors should support this case, and it should be working the same way as it works with regular vars, for the user.

Collaborator
nikic commented Dec 30, 2012

@smalyshev The question was basically whether you expect this to work to the same degree as it is with normal overloaded offsets and properties or in a more general way.

I.e. whether using &$foo->bar should only work when the get handler returns by reference. This is how it is for __offsetGet and __get. Otherwise you just get a warning that indirect modification of an overloaded property has no effect and it will basically behave as if you assigned $foo->bar rather than &$foo->bar. In my eyes that's what should be done here and it is how I interpreted what you said in that mail.

@cpriest on the other hand interpreted your message as asking this to work in the general case, even if the get accessor of $foo->bar does not return by reference. I.e. that some kind of proxy object should be returned in this case which should forward any gets/sets to the respective accessors.

So the question is basically about whether this is supposed to work with get not returning by reference ;)

@nikic yes, the way of implementing this seems to be using by-ref return on getter. However, this is not a requirement - the requirement is that it works, not how it works. Thus I did not include this specific implementation detail in the requirements.
If you implement it wrong - i.e., not using reference on getter - and there's no easy way to make it work - then the warning seems to be appropriate. If it's hard to do the warning, omitting it may be OK too. The necessary condition is that the correct code works, and incorrect code does not crash.

Owner
cpriest commented Dec 30, 2012

That's no problem then, if your getter returns a reference, this poses no issue.

Collaborator
nikic commented Dec 30, 2012

@smalyshev Great, then everything's working fine. Closing this.

@nikic nikic closed this Dec 30, 2012
Owner
cpriest commented Dec 31, 2012

Re-opening this. I had a .phpt stashed for the first 5, waiting on resolution of item 6 ambiguity, I pulled it out of the test case and item 6 does not work, getting 'Cannot assign by reference to overloaded object.'

In debugging I think the execution flow will change quite a bit once all accessors are actual properties as well, will re-address after those changes occurr.

@cpriest cpriest reopened this Dec 31, 2012
Collaborator
nikic commented Dec 31, 2012

@cpriest Cannot assign by reference to overloaded object. is okay in this situation. If it's easy enough to support set by reference (everywhere), we can fix it. If not, we don't necessarily have to as it matches the usual behavior ;)

Owner
cpriest commented Dec 31, 2012

I wonder if @smalyshev would agree. Right now it does run into that issue but I figured it could wait.

Owner
cpriest commented Jan 13, 2013

Closed by 57ffb19

Note, Cannot assign by reference to overloaded object. will still persist with accessors. Some sort of proxy is needed to support this language feature.

@cpriest cpriest closed this Jan 13, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment