parent::$foo with parent class after child class

[nikic]

Currently the implementation needs the ->parent CE of the active_class_entry to implement parent::$foo accesses. Normally this information is not available until runtime. To avoid it the implementation scans the opcodes for the ZEND_CLASS_FETCH instruction, extracts its name and looks it up in the class entry table.

This procedure fails if the extended class is only defined after the child class, as the following example shows:

<?php

class A extends B {
    public $foo {
        get { return 'parent::' . parent::$foo; }
    }
}

class B {
    public $foo {
        get { return 'foo'; }
    }
}

$a = new A;
var_dump($a->foo);

In this case an error is thrown:

 Fatal error: Access to undeclared static property: B::$foo in /home/nikic/dev/php-src/test.php on line 5

Even though static accessors won't be supported, this still has to be solved, because parent::$foo is used for normal properties as well.

[nikic]

I'm really not sure how this should be approached. This certainly can't be solved at compile time, so this will need to be done at runtime. But as the static property accesses don't have a read/write/exists separation I don't know how this should be done. One could add a special set of opcodes for parent::, though I think that's rather ugly. Maybe this will require to do the static accessors after all :/

[nikic]

By the way, this is obviously not limited to the class order example above. It happens whenever the parent class is not in the class table at the time of compilation of the child class.

E.g. if the class B would go in a different file and be included before the A class, then it would still fail (because the inclusion happens at runtime). Similarly if B would be autoloaded it wouldn't work either.

[cpriest]

This should be solvable for the non-static paradigm because
parent::__construct() and the ilk works just fine, it just needs to be
changed to operate the same was any any other parent:: access
works/happens. Since it's just a function call it's no different.

I think the parent:: issue you are running into will not be a problem
once the hacks I put in place to make static accessors work are removed.

On 12/29/2012 12:36 PM, nikic wrote:

By the way, this is obviously not limited to the class order example
above. It happens whenever the parent class is not in the class table
at the time of compilation of the child class.

E.g. if the class B would go in a different file and be included
before the A class, then it would still fail (because the inclusion
happens at runtime). Similarly if B would be autoloaded it wouldn't
work either.

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

-Clint

[nikic]

@cpriest I'm not sure I see what you mean. As parent::$foo isn't a method call, but a property access, it will use the usual opcodes for getting the static property. We can intercept it for this case, but the problem that read, write and isset are not distinguished stays the same as for normal static properties. Or am I missing something here?

[cpriest]

What I mean is this:

function __construct() {
parent::__construct();
...
}

Since this already works, there's no reason a parent::__getFoo();
shouldn't work exactly the same way.

On 12/29/2012 4:58 PM, nikic wrote:

@cpriest https://github.com/cpriest I'm not sure I see what you
mean. As |parent::$foo| isn't a method call, but a property access, it
will use the usual opcodes for getting the static property. We can
intercept it for this case, but the problem that read, write and isset
are not distinguished stays the same as for normal static properties.
Or am I missing something here?

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

[nikic]

@cpriest parent::__getFoo() will work, the problem is that we can't just generate a parent::__getFoo() call. This would break if parent::$foo were a normal static property. That's why you needed the active_class_entry->parent in the first place, to check whether it is an accessor or whether it is a normal property.

[cpriest]

I think we're on different pages... For non-static accessors there is
no conversion to __getFoo() at compile time, this is all handled through
zend_object_handler.c's function calls. At that point (runtime) we have
the active_class_entry and also the parent.

In any case, there is a conflict there b/w does the user want the
parent's static $foo or the accessor. The decision was already made
that accessors shadow properties, so I see no reason not to follow that
here. If the parent class has a $foo accessor, then parent::$foo would
call that, shadowing any static property that may exist. We could also
disallow the conflict during compile time as well by not allowing a
static $foo and a $foo accessor. Right?

On 12/30/2012 9:26 AM, nikic wrote:

@cpriest https://github.com/cpriest |parent::__getFoo()| will work,
the problem is that we can't just generate a |parent::__getFoo()|
call. This would break if |parent::$foo| were a normal static
property. That's why you needed the |active_class_entry->parent| in
the first place, to check whether it is an accessor or whether it is a
normal property.

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

[nikic]

In any case, there is a conflict there b/w does the user want the parent's static $foo or the accessor.

That's exactly the problem I see, that we can't distinguish between parent::$foo being a static property accessor or a accessor access. Consider this code:

<?php

class C extends P {
     public function doFoo() {
          parent::$foo = 'test';
     }
}

/* Case 1: Parent has an accessor with this name */
class P {
     public $foo {
          set { ... }
     }
}

/* Case 2: */
class P {
     public static $foo;
}

Here there could be two cases what the parent::$foo = 'test' means. Either it is supposed to invoke the parent::$foo accessor, it's supposed to write to the (static property) parent::$foo. At compile-time we don't know which of the cases it is, because we have no information about the parent.

Because we can't know at compile time we also can't do the parent::__setFoo(...) substitution trick. Instead a normal static property access is generated. We can check in the static property access whether the $foo is an accessor, but we wouldn't be able to invoke the set handler (as static properties don't know whether they are set or get).

Not sure whether this clarifies what I mean ^^

[cpriest]

I see what you mean, the normal, pre-accessor engine code converts parent::$foo to a static property access at compile time, is that right?

I think our only course of action then is to prevent, at compile time, this ambiguity by not allowing an accessor to be defined where a static property in the class chain exists. Then this ambiguity would be prevented, no?

Should we post this issue to the list and have them chime in here? Could create a lot of stalemate though.

[nikic]

@cpriest The issue is that we can't check the class chain at compile time. If we could, we could also directly check whether it is an accessor and not have the problem in the first place.

[cpriest]

What can we do about this then? Are you saying that at no point before the end of compilation this can be checked? That can't be right because the following code won't compile:

#!/usr/bin/phpd
<?php

class B extends A { 
    private $foo;
}

class A { 
    public $foo;
}

Fatal error: Access level to B::$foo must be public (as in class A) in /opt/php-core/git/trunk-accessor/cpriest/php/quick2.php on line 4

So at some point this should be resolvable.

Let me get the static code that I've put in pulled out and we can see what we can do at that point. I plan to locate the code which early-set the parent ce and remove that, it was just a few lines of code and I believe it was in the function that begins a class declaration.

[nikic]

@cpriest Those checks happen at run-time, not at compile time.

[cpriest]

Alright, I've found the code which does early-fetch and set of new_class_entry->parent and I don't see any real problem in leaving the code in, thought I'd get your opinion on that:

/* Locate parent_class_name and parent_class_entry to assign */
fetch_class = find_previous_op(ZEND_FETCH_CLASS TSRMLS_CC);

if(fetch_class != NULL && fetch_class->op2_type == IS_CONST) {
    zend_class_entry **parent_cepp = NULL;
    zval *parent_class_zv = &CG(active_op_array)->literals[fetch_class->op2.constant].constant;
    char *lc_parent_class = zend_str_tolower_dup(Z_STRVAL_P(parent_class_zv), Z_STRLEN_P(parent_class_zv));

    if (zend_hash_find(CG(class_table), lc_parent_class, strlen(lc_parent_class)+1, (void **) &parent_cepp)==SUCCESS) {
        new_class_entry->parent = *parent_cepp;
    }
    efree(lc_parent_class);
}

from d003093:5693

If it doesn't find a class by the given name it doesn't set it, so if it's available then it will be available earlier than before.

[nikic]

@cpriest If we don't need the code anymore it should be removed (as it can't be used in the general case anyway). Otherwise people will think that the parent CE is available, even though it really isn't.

[cpriest]

Alright, I'll remove it. One thing though, the error mentioned in a previous comment by me with "Fatal error: Access level to B::$foo must be public " is done at compile time, so we could disallow a static property w/ the same name during the compilation phase. I'm going to go forward with the idea that we will disallow a conflict b/w static $foo and accessor $foo throughout the inheritance chain.

[nikic]

@cpriest The check throws a compile error but does not actually happen at compile time. But that's not really important here, because for this purpose it doesn't matter when the check is done. I agree that we should check for this as it really doesn't make sense to override static with non-static or the other way around.

But I fail to see how this will fix the parent:: issue. This only makes sure that a property doesn't have different meaning through the inheritance chain, but we still won't know what kind of property it is (during compilation).

[cpriest]

Well you probably know the code better than I do, but I see that zend_do_inheritance() is called by zendparse() which means inheritance is checked at that time. So... w/ the parent::$foo issue I'll assume for the sake of argument that we let it compile as it typically would and let it become a stack of ZEND_FETCH_* and during inheritance check we can replace those opcodes with ZEND_DO_FCALL_BY_NAME.

Alternatively we could do it the other way and make it a function call referencing the parent.

I guess a 3rd option might possibly be to introduce a new opcode?

[cpriest]

Really need to decide on a solution to this, @smalyshev could you chime in here, could use some help from another expert. :)

One other possible solution is: https://gist.github.com/4420904

[cpriest]

Stas suggested parent->$foo which I think is perfect, I'll post this to the list and see what others think.

[smalyshev]

parent->$foo is not quite right - it means $foo is variable name, not foo. Yes, I know it's a mismatch between statics and dynamics, it's a problem :(

[cpriest]

oh right, duh. you wouldn't do $this->$foo... brain fart.

On 1/4/2013 12:05 AM, Stanislav Malyshev wrote:

parent->$foo is not quite right - it means $foo is variable name, not
foo. Yes, I know it's a mismatch between statics and dynamics, it's a
problem :(

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

[cpriest]

I'm good with parent->foo, there doesn't seem to be many complaints about it, nor alternatives posted.

[cpriest]

RFC Updated with Nikita's suggestion which met with no resistence.

(new ReflectionPropertyAccessor(CLASS, 'Milliseconds'))->setValue($this, $value);