Non-matching argument type for ArrayObject #3831

Closed
wants to merge 3 commits into
from

Conversation

Projects
None yet
3 participants
Contributor

RWOverdijk commented Feb 20, 2013

Documentation specifies that the accepted type is array|object. This only accepted arrays.
I haven't completely fixed the ArraObject in Stdlib yet. It's missing object support.

What I have done, however, is make it compatible. So remove array from the method. To catch other types I've added a conditional and I'm throwing an exception. I've added the unit test for these changes and they work as expected

@mwillbanks I assume that you'll build the object support seeing how it's your class? If not, I might be able to add it in a later PR

@weierophinney weierophinney commented on the diff Feb 20, 2013

tests/ZendTest/Stdlib/ArrayObjectTest.php
@@ -121,6 +121,15 @@ public function testExchangeArray()
$this->assertSame(array('bar' => 'baz'), $ar->getArrayCopy());
}
+ /**
+ * @expectedException InvalidArgumentException
+ */
+ public function testExchangeArrayStringArgumentFail()
+ {
+ $ar = new ArrayObject(array('foo' => 'bar'));
+ $old = $ar->exchangeArray('Bacon');
@weierophinney

weierophinney Feb 20, 2013

Owner

Please remove the annotation and use $this->setExpectedException(...) immediately prior to the statement that would raise the exception.

@RWOverdijk

RWOverdijk Feb 21, 2013

Contributor

This PR is rendered useless because @mwillbanks already fixed all of this in a different PR. But would you mind telling me why?

@weierophinney weierophinney commented on the diff Feb 20, 2013

library/Zend/Stdlib/ArrayObject.php
{
- $storage = $this->storage;
+ if (!is_array($data)) {
+ throw new Exception\InvalidArgumentException(
+ 'Invalid data type supplied. Expected array.'
+ );
+ }
@weierophinney

weierophinney Feb 20, 2013

Owner

I'm not sure I understand the rationale for this change; why is the typehint not sufficient here?

@mwillbanks

mwillbanks Feb 20, 2013

Contributor

He was attempting to make the change for the Session Container mostly and since the session container eventually proxies down to the ArrayObject he was attempting to not change the object.

@RWOverdijk

RWOverdijk Feb 21, 2013

Contributor

I was attempting to follow strict standards. The method declaration didn't match.

@weierophinney weierophinney commented on the diff Feb 20, 2013

library/Zend/Session/Container.php
{
+ if (!is_array($input) && !is_object($input)) {
@weierophinney

weierophinney Feb 20, 2013

Owner

Do we want to check against is_object, or instanceof ArrayAccess or method_exists($input, 'exchangeArray')? I'm pretty sure any object is not necessary going to work here...

@RWOverdijk

RWOverdijk Feb 21, 2013

Contributor

I'm pretty sure you're wrong. It doesn't require array access, nor does it require the method exchangeArray. I've checked the docs and php-src. It accepts any object or array. :)

Contributor

mwillbanks commented Feb 20, 2013

There was a reason why I did this and I cannot remember the exact reasoning; however, here is an example if you change the test for Stdlib\ArrayObject:

    public function testExchangeArray()
    {   
        $ar = new ArrayObject(array('foo' => 'bar'));
        $old = $ar->exchangeArray(array('bar' => 'baz'));

        $this->assertSame(array('foo' => 'bar'), $old);
        $this->assertSame(array('bar' => 'baz'), $ar->getArrayCopy());

        // show exchange array with ArrayObject
        $ar = new \ArrayObject(array('bar' => 'baz'));
        $ar2 = new \ArrayObject(array('foo' => 'bar'));
        $old = $ar->exchangeArray($ar2);

        $this->assertSame(array('bar' => 'baz'), $old);
        $this->assertSame(array('foo' => 'bar'), $ar->getArrayCopy());

        // attempt to do this with stdlib
        $ar = new ArrayObject(array('foo' => 'bar'));
        $ar2 = new ArrayObject(array('bar' => 'baz'));
        $old = $ar->exchangeArray($ar2);

        $this->assertSame(array('foo' => 'bar'), $old);
        $this->assertSame(array('bar' => 'baz'), $ar->getArrayCopy());
    }   

Note that how the Stdlib version will handle it; one way of doing it is to change the Stdlib\ArrayObject implementation to:

    public function exchangeArray($data)
    {   
        // handle arrayobject, iterators and the like:
        if (is_object($data) && method_exists($data, 'getArrayCopy')) {
            $data = $data->getArrayCopy();
        }   
        if (!is_array($data)) {
            $data = (array) $data;
        }   

        $storage = $this->storage;

        $this->storage = $data;

        return $storage;
    }   

However, now you would need to change the Zend\Session\Container\AbstractContainer exchangeArrayCompat method to bring it to the main one as it can work everywhere:

    public function exchangeArray($input)
    {   
        // handle arrayobject, iterators and the like:
        if (is_object($input) && method_exists($data, 'getArrayCopy')) {
            $input = $input->getArrayCopy();
        }   
        if (!is_array($input)) {
            $input = (array) $input;
        }   

        $storage = $this->verifyNamespace();
        $name    = $this->getName();

        $old = $storage[$name];
        $storage[$name] = $input;
        if ($old instanceof ArrayObject) {
            return $old->getArrayCopy();
        }   

        return $old;
    }   

Then in the Zend\Session\Container you need to remove the exchangeArray method.

This seems to all work; I'll check it out a bit more right now because if I remember right when making ArrayObject work this way there was something different about how the PHP \ArrayObject handles storing ArrayObjects; if I remember correctly the ArrayObject itself was simply set or anything traversable for that matter. Unfortunately this doesn't work the same way in userland. All of the tests check out but how this would actually work in the wild is a bit unknown.

@weierophinney weierophinney added a commit that referenced this pull request Feb 20, 2013

@weierophinney weierophinney Merge branch 'hotfix/3838'
Close #3838
Fixes #3831
99e22d7

@gianarb gianarb pushed a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015

@mwillbanks mwillbanks Resolved zendframework/zendframework#3831 made Stdlib\ArrayObject mor…
…e compatible with PHP ArrayObject
b039262

@weierophinney weierophinney added a commit to zendframework/zend-stdlib that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/3838' df43daf

@gianarb gianarb pushed a commit to zendframework/zend-session that referenced this pull request May 15, 2015

@mwillbanks mwillbanks Resolved zendframework/zendframework#3831 made Stdlib\ArrayObject mor…
…e compatible with PHP ArrayObject
5536ac3

@weierophinney weierophinney added a commit to zendframework/zend-session that referenced this pull request May 15, 2015

@weierophinney weierophinney Merge branch 'hotfix/3838' c3f023e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment