Wrong magic methods sequence with ?? operator

[nicolas-grekas]

Description

When ?? is used to check a property using magic methods, PHP unconditionnaly calls both __isset and __get even when __isset did actually set the property.

Reproducer: https://3v4l.org/cDlNW

This script:

#[AllowDynamicProperties]
class A
{
    public function __get($n)
    {
        echo __FUNCTION__, "\n";

        return $this->$n;
    }

    public function __isset($n)
    {
        echo __FUNCTION__, "\n";

        $this->$n = 123;

        return true;
    }
}

$a = new A;

echo $a->foo ?? 234;

echoes this:

__isset
__get
123

while it should echo this:

__isset
123

PHP Version

All since 7.0.6

Operating System

No response

[damianwadley]

__isset only confirms whether or not the "property" exists. That doesn't have to mean that the property exists and is not null.
So the call to __get is still necessary.

edit: Ah, you mean how the property gets created between the two calls.

[iluuu1994]

If you return false from __isset that's what happens. With true, the concrete value must be fetched. Edit: It seems you're asking that the existence of the property must be repeated after isset. I don't think isset should be used as a mechanism to create properties...

[nicolas-grekas]

Now seeing your edit @iluuu1994. But first of all yes the property should be fetched, but since __isset does set the property, the fetch shouldn't get through __get

I don't think isset should be used as a mechanism to create properties...

Nothing prevents it and that's actually the only way to properly implement lazy object so yes, it can :)

[iluuu1994]

I don't think there's a good reason to add another check. Can't you just return the property you create in __isset in __get? Adding a check slows down the 99+% case where __isset doesn't create a property.

[nicolas-grekas]

?? operation with magic methods is rare so that perf shouldn't be an issue. I also expect the call to be fast since it's just a missing hashmap check. And yes, this behavior prevents writing correctly behaving lazy objects - we cannot make them truly transparent.

[Girgias]

I must agree with @iluuu1994 here, __isset() shouldn't be used to assign properties, and adding a check is not something I am in favour of.

[nicolas-grekas]

I'm sorry but this doesn't make sense to me. The fact that __isset can be used to initialize properties is irrelevant to the issue. Actually, it's a critical capability that is leveraged since more than a decade to implement lazy objects. You might not be familiar with how those are implemented but the general idea is simple: all properties are unset when the object is created, and magic methods are added by inheritance to defer initialization to the moment when any property is accessed - that being via any magic methods, __isset included. There are critical piece of infrastructure that rely on lazy objects - many based on Doctrine entities and/or when using lazy services (eg Symfony DI container but not only it).

The current behavior just breaks the semantics of magic methods. They are supposed to be called only when a property isn't accessible, and the call to __get is just missing the check to guard this behavior.

Can you please expand on why you think things behave as expected? What's the argument to not fix this?

[bwoebi]

Currently __isset and __get are called as a pair: if (!exists()) { if (__isset()) return __get() else return null }.

Your proposal is to do if (!exists()) { if (!__isset()) return null; } if (!exists()) { return __get(); }.

This behaviour comes from the initial bugfix for PHP 7, it was probably done that way to just make it work, and maybe not the most fully thought out behaviour, but it's how it works today.

I don't think you should argue it would break critical pieces of infrastructure, otherwise they'd be long broken, since PHP 7 that is. @nicolas-grekas

I do however think, that from a theoretical perspective __get() should not be called, if it exists, period. It would be a break in long-standing behaviour though and as such only go into PHP 8.4, if this is changed.

I also don't think performance is a major concern in this case, as the relative overhead of __isset() and __get() calls must be far bigger than a simple undef check (for declared props) or a hash_exists (for dynamic props).

[iluuu1994]

Patch

diff --git a/Zend/tests/coalesce_is_create_property.phpt b/Zend/tests/coalesce_is_create_property.phpt
new file mode 100644
index 0000000000..10927b6289
--- /dev/null
+++ b/Zend/tests/coalesce_is_create_property.phpt
@@ -0,0 +1,22 @@
+--TEST--
+Coalesce IS fetch should repeat property existence check after __isset
+--FILE--
+<?php
+#[AllowDynamicProperties]
+class A {
+    public function __isset($prop) {
+        echo __FUNCTION__, "\n";
+        $this->$prop = 123;
+        return true;
+    }
+    public function __get($prop) {
+        throw new Exception('Unreachable');
+    }
+}
+
+$a = new A;
+echo $a->foo ?? 234;
+?>
+--EXPECT--
+__isset
+123
diff --git a/Zend/zend_object_handlers.c b/Zend/zend_object_handlers.c
index 6a22156d18..d2b34ff9bf 100644
--- a/Zend/zend_object_handlers.c
+++ b/Zend/zend_object_handlers.c
@@ -589,6 +589,36 @@ ZEND_API uint32_t *zend_get_property_guard(zend_object *zobj, zend_string *membe
 }
 /* }}} */
 
+static zval *zend_std_read_dynamic_property(zend_object *zobj, zend_string *name, void **cache_slot, uintptr_t property_offset)
+{
+	if (EXPECTED(zobj->properties != NULL)) {
+		if (!IS_UNKNOWN_DYNAMIC_PROPERTY_OFFSET(property_offset)) {
+			uintptr_t idx = ZEND_DECODE_DYN_PROP_OFFSET(property_offset);
+
+			if (EXPECTED(idx < zobj->properties->nNumUsed * sizeof(Bucket))) {
+				Bucket *p = (Bucket*)((char*)zobj->properties->arData + idx);
+
+				if (EXPECTED(p->key == name) ||
+					(EXPECTED(p->h == ZSTR_H(name)) &&
+						EXPECTED(p->key != NULL) &&
+						EXPECTED(zend_string_equal_content(p->key, name)))) {
+					return &p->val;
+				}
+			}
+			CACHE_PTR_EX(cache_slot + 1, (void*)ZEND_DYNAMIC_PROPERTY_OFFSET);
+		}
+		zval *retval = zend_hash_find(zobj->properties, name);
+		if (EXPECTED(retval)) {
+			if (cache_slot) {
+				uintptr_t idx = (char*)retval - (char*)zobj->properties->arData;
+				CACHE_PTR_EX(cache_slot + 1, (void*)ZEND_ENCODE_DYN_PROP_OFFSET(idx));
+			}
+			return retval;
+		}
+	}
+	return NULL;
+}
+
 ZEND_API zval *zend_std_read_property(zend_object *zobj, zend_string *name, int type, void **cache_slot, zval *rv) /* {{{ */
 {
 	zval *retval;
@@ -638,31 +668,9 @@ ZEND_API zval *zend_std_read_property(zend_object *zobj, zend_string *name, int
 			goto uninit_error;
 		}
 	} else if (EXPECTED(IS_DYNAMIC_PROPERTY_OFFSET(property_offset))) {
-		if (EXPECTED(zobj->properties != NULL)) {
-			if (!IS_UNKNOWN_DYNAMIC_PROPERTY_OFFSET(property_offset)) {
-				uintptr_t idx = ZEND_DECODE_DYN_PROP_OFFSET(property_offset);
-
-				if (EXPECTED(idx < zobj->properties->nNumUsed * sizeof(Bucket))) {
-					Bucket *p = (Bucket*)((char*)zobj->properties->arData + idx);
-
-					if (EXPECTED(p->key == name) ||
-				       (EXPECTED(p->h == ZSTR_H(name)) &&
-				        EXPECTED(p->key != NULL) &&
-				        EXPECTED(zend_string_equal_content(p->key, name)))) {
-						retval = &p->val;
-						goto exit;
-					}
-				}
-				CACHE_PTR_EX(cache_slot + 1, (void*)ZEND_DYNAMIC_PROPERTY_OFFSET);
-			}
-			retval = zend_hash_find(zobj->properties, name);
-			if (EXPECTED(retval)) {
-				if (cache_slot) {
-					uintptr_t idx = (char*)retval - (char*)zobj->properties->arData;
-					CACHE_PTR_EX(cache_slot + 1, (void*)ZEND_ENCODE_DYN_PROP_OFFSET(idx));
-				}
-				goto exit;
-			}
+		retval = zend_std_read_dynamic_property(zobj, name, cache_slot, property_offset);
+		if (retval) {
+			goto exit;
 		}
 	} else if (UNEXPECTED(EG(exception))) {
 		retval = &EG(uninitialized_zval);
@@ -693,6 +701,13 @@ ZEND_API zval *zend_std_read_property(zend_object *zobj, zend_string *name, int
 			}
 
 			zval_ptr_dtor(&tmp_result);
+
+			retval = zend_std_read_dynamic_property(zobj, name, cache_slot, property_offset);
+			if (retval) {
+				OBJ_RELEASE(zobj);
+				goto exit;
+			}
+
 			if (zobj->ce->__get && !((*guard) & IN_GET)) {
 				goto call_getter;
 			}

I agree with @bwoebi in that this is more of a missing feature than a bug. The "property fetch" is currently considered an atomic operation. zend_std_read_property does not actually consist of an "isset" and "get" phase. The properties value is fetched in the same step. Because there's no such equivalent in userland, we're forced to call both __isset and __get.

Anyway, returning false from __isset after creating a property is somewhat of a contradiction. We can act as if the property isn't there and call the ?? fallback (which normal properties cannot do, because they don't call __isset), or use the property anyway (which feels like it's going against what the user said). I went for the former, which is also what @bwoebi has expressed in his pseudo-code.

[nicolas-grekas]

I don't think you should argue it would break critical pieces of infrastructure

I made this point to answer the objection that "isset should[n't] be used as a mechanism to create properties..."

I agree that the reported behavior is an edge case. But it's one I stumbled upon while working on lazy objects so it's clearly a defect in how the engine behaves currently.

To me it's an edge case that won't affect anyone negatively in practice and I would consider it as a bug fix. But I don't mind patching this in 8.4 if you think that's how it should be done.

returning false from __isset after creating a property is somewhat of a contradiction

Unless the value of the property is null! (note that the reported case is about __isset returning true)

Thanks for the patch, I'm going to play with it right away.

[iluuu1994]

It turns out the patch doesn't work for declared, unset properties, as @bwoebi has pointed out. I think the might be soundness issues in that case too, because __isset may free the object, invalidating the pointer. As the fix might not be as trivial as expected, I'd definitely prefer waiting for 8.4.

[dstogov]

This can't be fixed without a behaviour change.
Personally, I think this shouldn't be changed.
Otherwise we will have a segmentation with different behaviours before and after PHP-8.4 and "lazy properties" will have to support both ones.

In case the __isset() callback is used in unexpected way, it should be also possible to implement __get() supporting the existing engine behaviour.

[iluuu1994]

@dstogov I think the argument is that lazy objects cannot transparently handle this case. @nicolas-grekas It would be nice if you could provide a minimal example of what case you cannot solve without this fix by overriding __get.

[nicolas-grekas]

Here is a simplified example :

<?php

// This is the class that we want to make lazy.
// Note that this is an opaque class from the PoV of the proxy-generation logic:
// its implementation details are unknown.
class A
{
    protected array $data = [];

    public function __construct()
    {
        $this->data = ['foo' => 123];
    }

    public function __get($n) { return $this->data[$n] ?? null; } // line 15
    public function __set($n, $v) { $this->data[$n] = $v; }
    public function __isset($n) { return isset($this->data[$n]); }
    public function __unset($n) { unset($this->data[$n]); }
}

// This is a simplified proxy class that would typically be generated automatically
// from the signature of the proxied class. The way this works is by unsetting all
// declared properties at instantiation time, then using the magic methods to lazily
// initialize them on demand. This is a very simple implementation that does not handle
// all edge cases, but it should be enough to illustrate the issue. The logic in each
// magic method is very repetitive: we checks if we're trying to access the uninitialized
// property. If yes, we initialize it, if not, we call the parent method.
class LazyA extends A
{
    private const STATUS_UNINITIALIZED = 0;
    private const STATUS_INITIALIZING = 1;
    private const STATUS_INITIALIZED = 2;

    private $lazyStatus = self::STATUS_UNINITIALIZED;

    public function __construct()
    {
        unset($this->data);
    }

    public function __get($n)
    {
        if (self::STATUS_INITIALIZED === $this->lazyStatus || 'data' !== $n) {
            return parent::__get($n);
        }

        $this->initialize();

        return $this->data;
    }

    public function __set($n, $v)
    {
        if (self::STATUS_INITIALIZED === $this->lazyStatus || 'data' !== $n) {
            parent::__set($n, $v);
        }

        $this->initialize();

        $this->data = $v;
    }

    public function __isset($n)
    {
        if (self::STATUS_INITIALIZED === $this->lazyStatus || 'data' !== $n) {
            return parent::__isset($n);
        }

        $this->initialize();

        return isset($this->data);
    }

    public function __unset($n)
    {
        if (self::STATUS_INITIALIZED === $this->lazyStatus || 'data' !== $n) {
            parent::__unset($n);
        }

        $this->initialize();

        unset($this->data);
    }

    private function initialize()
    {
        if (self::STATUS_INITIALIZING === $this->lazyStatus) {
            return;
        }

        $this->lazyStatus = self::STATUS_INITIALIZING;
        parent::__construct();
        $this->lazyStatus = self::STATUS_INITIALIZED;
    }
}

$a = new LazyA();

// Currently, this will trigger a TypeError:
// Cannot assign null to property A::$data of type array on line 15
// With Ilija's patch, this will work as expected and will display int(123)
var_dump($a->foo);

[nicolas-grekas]

BTW, I confirm that with @iluuu1994's patch the test cases I was working on are all green: I'm able to fix a buggy behavior that I had to implement as a workaround for the current behavior.

[dstogov]

The error message TypeError: Value of type null returned from A::__get() must be compatible with unset property A::$data of type array looks weird, because it occurs when your program tries to read property "data" (that is unset) and you return null. The error message is correct.

This may be fixed by modifying the line 15

    public function __get($n) { return $this->data[$n] ?? ($n == "data" ? [] : null);} // line 15

[nicolas-grekas]

We cannot do that because that would break encapsulation at two levels:

1. class A couldn't store a virtual property named data anymore
2. the proxy class is meant to decorate any class without knowing anything about its implementation. Requiring such behavior from decorated classes would create some sort of coupling, which must be avoided.

The issue is really the engine not behaving as it should - the rule is simple and not respected with the ?? operator. It should be fixed, that's all. What's the issue with fixing this behavior?

[nicolas-grekas]

Just to be sure we're on par: do we want to fix this as a bug or do we want to go through an RFC?

[dstogov]

Just to be sure we're on par: do we want to fix this as a bug or do we want to go through an RFC?

I'm not sure.
This is not a simple bug and the behavior change can't be committed into the old branches.
#12698 doesn't fix the general inconsistency. It may be committed to master only, but this won't help a lot.
I don't see a good solution for fixing the inconsistency and most probably it's not going to be found soon.
I would prefer to keep this as is, instead of introducing more mess, but I may be wrong.

[Girgias]

I will also note, that ?? is not equivalent to isset() and the ternary operator for when one tries to access invalid string offsets (related to #7173) https://3v4l.org/aDLLB

[nicolas-grekas]

@Girgias you're right but this also spans to other operators, eg &=. While it could be expected to have these work on string offsets seamlessly, that's not the case, and I would consider this a separate topic.

[mvorisek]

In my opinion x ?? y should be strictly equivalent to isset(x) ? x : y. This is how it is documented, RFC-accepted and how it is the easiest to understand.

[iluuu1994]

@nicolas-grekas Are you planning on moving this discussion forward? Or did you find an acceptable solution for your use-case?

[nicolas-grekas]

Yes, I'd like this to be fixed so I'll raise the issue ASAP on internals, thanks for the reminder.

[iluuu1994]

@nicolas-grekas Is this still relevant with the introduction of lazy objects?

[nicolas-grekas]

Yes it is.

But I'm also thinking about an RFC that'd add an __exists magic method, that'd help resolve the ambiguity we have between null and unset properties at the moment.

I'd be fine to properly define the behavior of ?-operators when this method exists, and leaving the current behavior as is otherwise. I don't understand why we'd need to preserve BC for a broken behavior but at least that's a plan forward ;)

[iluuu1994]

I don't really care about BC here, these are extreme edge cases. But object handlers are already very complex, things like this don't make them simpler.

Yes it is.

Can you clarify why?

[nicolas-grekas]

Nothing fancy - I just mean the initial report: The sequence is incorrect currently. I'm not going to fall into it anymore with lazy-objects but that still remains incorrect behavior :)

[iluuu1994]

Thank you for the clarification. I'm going to close my PR in favor of lazy objects then. I'm not sure if the current behavior can be classified as "wrong", but with an available alternative now, it doesn't seem necessary to modify it.