WeakMap::offsetExists return false if value is null

[julien-boudry]

Description

The following code:

<?php

$wm = new \WeakMap;
$os = new \SplObjectStorage;

$obj1 = new \stdClass;
$obj2 = new \stdClass;

$wm[$obj1] = null;
$wm[$obj2] = true;

$os[$obj1] = null;
$os[$obj2] = true;

var_dump($wm->offsetExists($obj1));
var_dump($wm->offsetExists($obj2));

var_dump($os->offsetExists($obj1));
var_dump($os->offsetExists($obj2));

foreach ($wm as $key => $value) {
    if ($key === $obj1) {
        echo 'key $obj1 is always here';
    }
}

Resulted in this output:

bool(false)
bool(true)
bool(true)
bool(true)
key $obj1 is always here

But I expected this output instead:

bool(true)
bool(true)
bool(true)
bool(true)
key $obj1 is always here

PHP Version

PHP 8.1.5

Operating System

*

[cmb69]

https://github.com/php/php-src/blob/e063243d2e0d3c17a8703cb59b4fd1fcb6a7a9cd/Zend/zend_weakrefs.c#L384

I wonder if that is deliberate or if s/IS_NULL/IS_UNDEF would be more appropriate.

[nikic]

This is deliberate. Despite the confusing name, the API contract of offsetExists is that it must return false for null values. It is the analogue of __isset() and must match isset() semantics.

[julien-boudry]

This is deliberate. Despite the confusing name, the API contract of offsetExists is that it must return false for null values. It is the analogue of __isset() and must match isset() semantics.

Will other Spl implementations of ArrayAccess, like SplObjectStorage, will change in the future according to this?

[julien-boudry]

• Note that, in this example, var_dump( isset($os[$obj1]) ); return true.

[cmb69]

Despite the confusing name, the API contract of offsetExists is that it must return false for null values. It is the analogue of __isset() and must match isset() semantics.

Ah, right.

Will other Spl implementations of ArrayAccess, like SplObjectStorage, will change in the future according to this?

SplObjectStorage::offsetExists() is an alias of ::contains(), what is apparently a deliberate design decision: https://github.com/php/php-src/blob/7ce5e0a8f166142f64edfc2cd1f92c45b9c0b946/ext/spl/spl_observer.c#L459

I think we should clarify the respective note in the manual.

Are there other classes, where ::offsetExists() exhibits this behavior?

[julien-boudry]

I think we should clarify the respective note in the manual.

Also on ArrayAccess manual, is unclear ( https://www.php.net/manual/en/arrayaccess.offsetexists.php ) even if it's talking about the empty() function

[MorganLOCode]

Just encountered this myself (gettype($splos[$i]) is NULL (actually, never assigned to) but isset($splos[$i]) is true), and found nothing to suggest the behaviour was correct.

The only part of the manual I found that was relevant was the statement on the isset manual page that it returns false for a variable that has been assigned null.

Is it correct? If it's a deliberate decision, what was the rationale? Or is it just a emergent consequence of offsetExists/contains being aliased together?

[damianwadley]

Just encountered this myself (gettype($splos[$i]) is NULL (actually, never assigned to) but isset($splot[$i]) is true), and found nothing to suggest the behaviour was correct.

I can't reproduce this. https://3v4l.org/NKKhE

Additionally, "never assigned to" suggests you didn't create $i in the $splot WeakMap, in which case gettype-ing it will throw an exception because it doesn't exist, so I'm not sure what you mean by that.

Unless, of course, the splos/splot typo in your comment is also present in your code?

[MorganLOCode]

Try using SplObjectStorage instead of WeakMap per the comment from @cmb69 above. The issue seems more generally around the behaviour of offsetExists. I'll fix that typo.

In perhaps related behaviour, isset($storage[$unsetvar]) will spit warnings when $unsetvar is not initialised; that case is not dealt with before passing it off to the offsetExists method. Would moving unset/null tests up so they're done ahead of calling SPL methods be a way of addressing these in a general sense?

[iluuu1994]

I had a quick look at that. Unfortunately, there attach method deliberately uses NULL as an empty value. So, changing the semantics to match isset means we'll also need to change this empty value. There's also this comment:

https://github.com/php/php-src/blob/360e6f842c885f9e6018f603ba00ceb5cd78ef31/ext/spl/spl_observer.c#L457

So this seems intentional, although dubious.

[MorganLOCode]

In perhaps related behaviour, isset($storage[$unsetvar]) will spit warnings when $unsetvar is not initialised

Not sure why I thought this was a problem when I wrote that: it's always been normal behaviour. The test is whether the indexed element exists or not - which assumes the index exists.