nikic / scalar_objects

Extension that adds support for method calls on primitive types in PHP
MIT License
1.13k stars 44 forks source link

Refactor: Handler instances self param #12

Closed thekid closed 9 years ago

thekid commented 10 years ago

This pull request refactors the API to pass the value the method is invoked on as first argument to handlers' methods instead of filling in $this with it. This way, we're able to overcome the limitation of not being able to modify the value in PHP >= 5.6.0 (making the array_pop example work once again) and gets rid of segmentation faults when PHP relies on $this being an object.

Old

class ArrayHandler {
    public function pop() {
        return array_pop($this);
    }
}
$array= [1, 2, 3];
var_dump($array->pop()); // 3
var_dump($array);        // [1, 2, 3]

New

class ArrayHandler {
    public function pop(&$self) {
        return array_pop($self);
    }
}
$array= [1, 2, 3];
var_dump($array->pop()); // 3
var_dump($array);        // [1, 2]
thekid commented 10 years ago

Unfortunately, I can't find a working alternative for num_additional_args for PHP versions < 5.6.0. If I search for the ZEND_FCALL opcode and modify it like this:

 /* Find the fcall opcode, its extended_value holds the number of arguments */
{
    zend_op *execute_op = execute_data->opline;
    while (ZEND_DO_FCALL_BY_NAME != execute_op->opcode) {
        execute_op++;
    }
    execute_op->extended_value++;
}

It seems to work alright but then gives me a segfault after calling into the handler the second time around. The test "basic-repeated.phpt" added in thekid/scalar_objects@0add4c9730bf5a212d3612db2a35358d8be87605 shows this.

An option to solve this could be to remove PHP 5.4 and 5.5 support.

nikic commented 10 years ago

This doesn't really solve the issue with the by-ref modification - it is independent of how the method is called / the value passed. The issue is that in 5.6 it is no longer possible (as far as I could determine) to do a zval_ptr_ptr fetch on the object of a method call. We can fetch it for an IS_CV operand, but not for IS_VAR. The reason is that FETCH_DIM_R no longer does an AI_SET_PTR.

Just doing a SET_ISREF_P doesn't solve this - if the zval was shared this will make it a reference for everybody, not just this particular usage.

Moving to an argument does resolve the numerous issues that can arise from $this not being an object - however it isn't compatible with PHP<5.6 (and I'd say inherently so). I don't think it's really viable to only support PHP 5.6 (which isn't even released) here.

nikic commented 10 years ago

For PHP <5.6 we could go through an extra indirection: Instead of calling the method directly, we call an internal dummy function which then calls the actual method with an extra parameter.

This is similar to what PHP does for __callStatic, for example.

nikic commented 9 years ago

I've recently updated the ext to use a calling convention with an extra param. I ended up going with a VIA_HANDLER indirection function for all versions, because otherwise arginfo checks won't be correct (also there's a very unlikely, but possible, issue when the call happens exactly at a VM stack page boundary).

I've dropped support for by-reference fetching of the "object" for all versions. It turns out this didn't fully work even in PHP 5.4 and 5.5, see https://github.com/nikic/scalar_objects/issues/13#issuecomment-52553553.