nikic / scalar_objects

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

Anonymous callback functions in array_* functions #10

Closed deeky666 closed 9 years ago

deeky666 commented 10 years ago

I can't get an array handler to work with anonymous callback functions defined inside the handler directly like:

<?php

register_primitive_type_handler('array', 'ArrayHandler');

class ArrayHandler
{
    public function implode($delimiter = ', ', $enclosure = null)
    {
        $array = $this;

        if (null !== $enclosure) {
            $array = $this->map(
                function ($value) use ($enclosure) {
                    return $enclosure . $value . $enclosure;
                }
            );
        }

        return implode($delimiter, $array);
    }

    public function map($function)
    {
        return array_map($function, $this);
    }
}

$array = array('foo', 'bar');

$strtoupper = function ($value) {
    return strtoupper($value);
};

var_dump($array->map('strtoupper')); //works
var_dump($array->map($strtoupper));  // works

var_dump($array->implode());  // works
var_dump($array->implode(', ', '"'));  // causes script to hang

Test

deeky@98N-MUELLER-STEVE:~/$ php -f scalar_objects.php 
array(2) {
  [0]=>
  string(3) "FOO"
  [1]=>
  string(3) "BAR"
}
array(2) {
  [0]=>
  string(3) "FOO"
  [1]=>
  string(3) "BAR"
}
string(8) "foo, bar"
^C^C^CGetötet

The last line shows that I tried to abort the process but infact I had to kill it with kill -9 :(

What seems to be not working is defining anonymous functions inside a handler's method whereas passing an anonymous function from outside into the method works without problems.

Is there any chance to fix that? I Would like to make use of this extension for a project.

deeky666 commented 10 years ago

Ah before I forget:

deeky@98N-MUELLER-STEVE:~/$ php -v
PHP 5.5.3-1ubuntu2.1 (cli) (built: Dec 12 2013 04:24:35) 
Copyright (c) 1997-2013 The PHP Group
Zend Engine v2.5.0, Copyright (c) 1998-2013 Zend Technologies
    with Zend OPcache v7.0.3-dev, Copyright (c) 1999-2013, by Zend Technologies
nikic commented 10 years ago

The issue is caused by $this not being an object, but the closure expecting it to be (and invoking object specific macros).

As a temporary solution you can disable $this-binding by making the closure static, e.g:

            $array = $this->map(
                static function ($value) use ($enclosure) {
                    return $enclosure . $value . $enclosure;
                }
            );
nikic commented 10 years ago

This doesn't look easy to fix... I think the message is that I should get away from using $this and instead pass the array/string/... as the first argument to the method. The use of $this is just too much magic that was bound to cause issues somewhere (the somewhere turned out to be closures).

I'd switch to using the first argument approach, but due to the stack allocation optimizations in PHP 5.5 this may be rather complicated to implement. Gone are the good times where you could just push an argument to the stack...

I'll need to think on this a bit more.

deeky666 commented 10 years ago

@nikic Thank you for the quick reply. I get the problem. I'm not too familiar with C code and PHP internals but I was wondering whether calling the handler methods actually happens on "real" objects, so to say are you instantiating an object when invoking the handler? I guess that is not the case because of the rather magic $this implementation. Still, if I'm wrong and you actually have real objects, what about passing the value to the constructor? Otherwise your suggestion might be okay although always passing the value as first argument is also somewhat strange API ;)

deeky666 commented 10 years ago

Btw: Thank you so much for this extension, this in conjunction with a self-made patch for scalar type hints will save my life when needing to deal with strict typing in private projects :)

nikic commented 10 years ago

Could you please test whether the issue is resolved now? I added a bit of code to avoid binding $this for closures in the type handler. Travis says the segfault is still there, but I can't reproduce it locally, so would be nice to know whether it works for you.

The idea about passing as the first arg turned out to be a dead end - I forgot that I only added the necessary APIs to add arguments during runtime in PHP 5.6 :/

Regarding your question: No, an object isn't created for this. Doing so would be rather slow and there would later be issues turning that object back to the primitive type automatically.

deeky666 commented 10 years ago

@nikic Thank you so much for fixing this :) Works perfectly fine now for me. Maybe it still fails on Travis because it uses PHP 5.4 instead of PHP 5.5? You should try adding more PHP versions to the build matrix. This could show up whether the bug is bound to a specific PHP version. However I am already using this extension a lot already in conjunction with a custom PHP source where I patched primitive type hint support in and it is an awesome coding experience so far :) Of course I can only use this for private projects but it shows up that an API approach on primitive types like yours is REALLY helpful and makes code much more readable, understandable and less error prone. In conjunction with primitive type hints you can really focus on important things while coding without having to if !is_*() throw \InvalidArgumentException which makes PHP fun again :)

nikic commented 10 years ago

@deeky666 Would you mind posting the APIs that you are using with this somewhere? Interesting to know what other people came up with ;)