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

__get() can't recursively call itself #254

Closed
Deswal opened this issue Dec 3, 2010 · 8 comments
Closed

__get() can't recursively call itself #254

Deswal opened this issue Dec 3, 2010 · 8 comments

Comments

@Deswal
Copy link

@Deswal Deswal commented Dec 3, 2010

class foo {
function __get($v) {
  switch($v) {
    case 'foo':
      return $this->bar;
      break;
    case 'bar':
      return 'bar';
      break;
    }
  }
}

$foo = new foo();
echo($foo->foo);

hphp output:
HipHop Notice: Undefined property: foo::$bar
php output:
bar

@ftzdomino
Copy link

@ftzdomino ftzdomino commented Jan 15, 2011

Here's a much smaller script to demonstrate this issue:

class foo {
function __get($v)
{
switch($v) {
case 'foo':
return $this->bar;
break;
case 'bar':
return 'bar';
break;
}
}
}

$foo = new foo();
echo($foo->foo);

hphpi output:
HipHop Notice: Undefined property: foo::$bar
php output:
bar

@ftzdomino
Copy link

@ftzdomino ftzdomino commented Jan 15, 2011

The problem appears to be that in src/runtime/base/object_data.cpp. The UseGet attribute does not take into account the name of the __get() attribute that are you fetching. It seems like in order to fix this you'd have to keep a stack of attributes that you are running __get() on which could be a major performance hit.

@ptarjan
Copy link
Contributor

@ptarjan ptarjan commented May 13, 2013

We're closing out all bugs older than 2 months. http://www.hiphop-php.com/wp/?p=575

If this is still an issue, please re-open it, and in order of goodness:

  1. Give detailed repro steps
  2. Write a test case in hphp/tests/quick (run it with hphp/tests/run) and send the pull request
  3. Fix it in a pull request
@ptarjan ptarjan closed this May 13, 2013
@sandeepone
Copy link

@sandeepone sandeepone commented Jun 14, 2013

I still see this issue, it's not fixed. Please reopen and fix it.

@ptarjan ptarjan reopened this Jun 14, 2013
@karpa13a
Copy link

@karpa13a karpa13a commented Jun 25, 2013

me too

@sandeepone
Copy link

@sandeepone sandeepone commented Jun 25, 2013

The patch seems to work for me, you need to patch manually.

@scannell
Copy link
Contributor

@scannell scannell commented Jun 25, 2013

We're aware of the issue. The attached patch introduces a different bug as @ftzdomino stated which is why we haven't pulled it into HHVM. (See #384, a dupe of this, for more discussion.)

@scannell
Copy link
Contributor

@scannell scannell commented Jul 22, 2013

This isn't on our roadmap to fix so I've marked this 'wishlist' for now. If the priority of this changes, we will investigate, and if someone provides an acceptable pull request, we'll merge it, but otherwise this is unlikely to be fixed any time soon.

@scannell scannell closed this Jul 22, 2013
crgwbr added a commit to silvermine/hhvm that referenced this issue Sep 2, 2013
This creates the possibility of infinite-loop recursion if used messily, but
fixes an Undefined Property bug occuring when a getter tried to re-enter itself
with a different property name. See Issues:

- facebook#254 (facebook#254)
- facebook#348 (facebook#384)
- facebook#657 (facebook#657).
crgwbr added a commit to silvermine/hhvm that referenced this issue Oct 23, 2013
This creates the possibility of infinite-loop recursion if used messily, but
fixes an Undefined Property bug occuring when a getter tried to re-enter itself
with a different property name. See Issues:

- facebook#254 (facebook#254)
- facebook#348 (facebook#384)
- facebook#657 (facebook#657).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.