In HHVM function array_key_exists returns FALSE for ArrayObjects

[Verber]

Discovered when running self-tests of PHPunit 3.7

Test method Framework_AssertTest::testAssertArrayHasKeyAcceptsArrayAccessValue

/**
 * @covers PHPUnit_Framework_Assert::assertArrayHasKey
 */
public function testAssertArrayHasKeyAcceptsArrayAccessValue()
{
    $array = new ArrayObject();
    $array['foo'] = 'bar';
    $this->assertArrayHasKey('foo', $array);
}

Constraint PHPUnit_Framework_Constraint_ArrayHasKey($key) use array_key_exists function to check that key exists:

/**
 * Evaluates the constraint for parameter $other. Returns TRUE if the
 * constraint is met, FALSE otherwise.
 *
 * @param mixed $other Value or object to evaluate.
 * @return bool
 */
protected function matches($other)
{
    return array_key_exists($this->key, $other);
}

In HHVM this function returns FALSE for ArrayObjects

[Verber]

Sorry, forgot to add HHVM version info I used Ubuntu 13.04 package

hhvm --version                                           
HipHop VM v2.2.0 (rel)
Compiler: heads/master-0-g431ef81e51b015db71ac35195ab837480503635f
Repo schema: b3632a5f67c4172817a5017fb18eeb1a3b5be51a

[paroski]

Thanks for reporting this! Feel free to submit a PR if you (or someone else reading this) gets to this before we do.

[scannell]

I should add that PHPUnit unit tests is one of the things we're looking at so we will likely get to this sometime in the coming months if no one else does beforehand for us.

[Verber]

Sorry for silly question I'm trying to find out how to compile hhvm with debug symbols, to run Eclipse CDT debug. Is it possible at all? I mean debug cpp code inside hhvm?

[scannell]

I believe cmake -DCMAKE_BUILD_TYPE=Debug (and then rebuilding with make) is what you want?

[Verber]

Seems yes, thank you!

[Verber]

I've done some research and now Im confused a bit: maybe this issue is irrelevant and all we should do there just fix PHPUnit, because ArrayObject is not an array indeed. For me it's looks like a bug or undocumented option in PHP, because documentation says that array_keyexists should accept only variables of array type as second argument, object support is there only for backward compatibility and may work only with public properties. Current PHP behavior looks strange also because other array* functions applied to ArrayObject produces warning, for example array_keys What do you think about this inconsistency? Should we close this issue?

[scannell]

(CC @paroski)

I'm personally not anxious to mirror unintended, unintuitive behavior unless it is blocking.

@sebastianbergmann, what do you think? (Are you emotionally invested in ArrayObject effectively silently casting in the case of array_key_exists?)

[sebastianbergmann]

I did not write testAssertArrayHasKeyAcceptsArrayAccessValue and do not know what the intention of the test is right now. Maybe @edorian or @whatthejeff can shed some light.

[whatthejeff]

@sebastianbergmann Looks like it comes from sebastianbergmann/phpunit#295.

[whatthejeff]

Would be nice to hear from @edorian since he accepted this PR, but i don't think we should rely on this functionality in PHPUnit. The array_key_exists documentation states:

Note: For backward compatibility reasons, array_key_exists() will also return TRUE if key is a property defined within an object given as array. This behaviour should not be relied upon, and care should be taken to ensure that array is an array.

To preserve backwards compatibility, we could explicitly cast the ArrayObject to an array before calling array_key_exists.

[Verber]

@sebastianbergmann @whatthejeff If you not mind I can fix this constraint in PHPUnit

[whatthejeff]

@Verber sounds good to me!

[whatthejeff]

I've done some research and now Im confused a bit: maybe this issue is irrelevant and all we should do there just fix PHPUnit, because ArrayObject is not an array indeed. For me it's looks like a bug or undocumented option in PHP, because documentation says that array_keyexists should accept only variables of array type as second argument, object support is there only for backward compatibility and may work only with public properties. Current PHP behavior looks strange also because other array* functions applied to ArrayObject produces warning, for example array_keys What do you think about this inconsistency? Should we close this issue?

@Verber array_key_exists was originally called key_exists and worked on arrays and objects. (see php/php-src@34f03f2c5cce5b668bfaa1ec04ef8d2f1e9c33a1). ArrayObject is a little unique in that it overrides the underlying methods called by HASH_OF() (and also convert_to_array()) to return the underlying array. So, in essence, this works by coincidence and we probably shouldn't rely on it.

[scannell]

For the moment I'm going to wontfix this based on the discussion here. @Verber, thanks for following up in PHPUnit!

[edorian]

Hi,

accepting the pull request back then (if memory serves me right) didn't have anything to do with the bc-behavior of array_key_exists

For backward compatibility reasons, array_key_exists() will also return TRUE if key is a property defined within an object given as array. This behaviour should not be relied upon, and care should be taken to ensure that array is an array.

but was done because of the ArrayAccess interface.

ArrayAccess::offsetExists

When using empty() ArrayAccess::offsetGet() will be called and checked if empty only if ArrayAccess::offsetExists() returns TRUE.

Me and (iirc) the person how made the PR where under the impression that those behaviors (accessing keys on an ArrayObject) should work with isset and array_key_exists because it would just kinda make sense.

I know PHP is really weird with its split between arrays and objects and the quite many edge cases when dealing with the two but I put the test there to be sure that basic things that work on arrays also work with implements ArrayAccess because that's how i think it should work and how the PHPUnit assert method should work (I.e. say "make sure i can call $foo['x'] and get the value.".

I'm not against changing the implementation of PHPUnit to make the check work with ArrayAccess objects explicitly though.

I might also be completely off with my assumptions but this is why this was done that way :)

[edorian]

And since this obviously doesn't work using ArrayAccess ( http://3v4l.org/Cl2ri ) because it doesn't call offsetExists internally I'd say we just throw this out of PHPUnit (or trigger a BC warning)

[whatthejeff]

@edorian yeah, array_key_exists is essentially using zend_symtable_exists(HASH_OF(array), ...) which is very similar (with a few exceptions) to casting the first argument to an array.

[scannell]

From HHVM's side we'd rather be explicit about semantics we support -- in some cases that means we're going to end up implementing undefined but widely relied upon PHP behavior but we're trying to limit the number of changes we do of that type [and unfortunately we have not taken on writing a formal spec for PHP at least at present.]

[whatthejeff]

@scannell Might be nice to have something like rubyspec for PHP.

[rwos]

FYI: above property of array_key_exists is also used in ZendFramework 1, to work around a rather ancient PHP bug:

• https://github.com/zendframework/zf1/blob/master/library/Zend/Registry.php#L206
• https://bugs.php.net/bug.php?id=40442

The result is that code using Zend_Registry doesn't work with hhvm. For better or worse, this includes pretty much all ZF1 projects, I'd guess. I think that hints into the direction that it is in fact widely relied upon behavior, no?

[scannell]

I don't think we've even looked at ZF1 (we've been focusing on compatibility with ZF2.)