Fix GH-12695: skip __get() when __isset() materialised the property

NEWS

@@ -21,6 +21,8 @@ PHP                                                                        NEWS
   . Deprecate specifying a nullable return type for __debugInfo(). (timwolla)
   . Fixed bug GH-22142 (Assertion failure in zendi_try_get_long() on IS_UNDEF).
     (David Carlier)
+  . Fixed GH-12695 (Wrong magic methods sequence with ?? operator).
+    (nicolas-grekas)
 
 - BCMath:
   . Added NUL-byte validation to BCMath functions. (jorgsowa)

UPGRADING

@@ -19,6 +19,11 @@ PHP 8.6 UPGRADE NOTES
 1. Backward Incompatible Changes
 ========================================
 
+- Core:
+  . ??/empty() on a magic property no longer call __get() when __isset()
+    has materialised the property by writing into the property table.
+    The freshly-written value is returned directly. isset() is unaffected.
+
 - DOM:
   . Properties previously documented as @readonly (e.g. DOMNode::$nodeType,
     DOMDocument::$xmlEncoding, DOMEntity::$actualEncoding, ::$encoding,

Zend/tests/magic_methods/gh12695.phpt

@@ -0,0 +1,46 @@
+--TEST--
+GH-12695: __get() is not called under `??` when __isset() materialised the property
+--FILE--
+<?php
+
+#[AllowDynamicProperties]
+class A {
+    public function __get($n) {
+        throw new Exception("__get must not be called when __isset materialised the property");
+    }
+    public function __isset($n) {
+        echo "  __isset($n)\n";
+        $this->$n = 123;
+        return true;
+    }
+}
+
+echo "Dynamic property materialised in __isset, then `??`:\n";
+$a = new A;
+var_dump($a->foo ?? 'fallback');
+
+echo "\nSame on a declared (unset) property:\n";
+class B {
+    public int $x = 99;
+    public function __get($n) {
+        throw new Exception("__get must not be called when __isset materialised the property");
+    }
+    public function __isset($n) {
+        echo "  __isset($n)\n";
+        $this->$n = 7;
+        return true;
+    }
+}
+$b = new B;
+unset($b->x);
+var_dump($b->x ?? 'fallback');
+
+?>
+--EXPECT--
+Dynamic property materialised in __isset, then `??`:
+  __isset(foo)
+int(123)
+
+Same on a declared (unset) property:
+  __isset(x)
+int(7)

Zend/tests/magic_methods/gh12695_empty.phpt

@@ -0,0 +1,55 @@
+--TEST--
+GH-12695: empty() also benefits from the post-__isset() materialization re-check
+--FILE--
+<?php
+
+#[AllowDynamicProperties]
+class A {
+    public function __get($n) {
+        throw new Exception("__get must not be called when __isset materialised the property");
+    }
+    public function __isset($n) {
+        echo "  __isset($n)\n";
+        $this->$n = $GLOBALS['next_value'];
+        return true;
+    }
+}
+
+echo "1) empty() when __isset materialised a truthy value: __get is not called, empty=false:\n";
+$GLOBALS['next_value'] = 7;
+$a = new A;
+var_dump(empty($a->foo));
+
+echo "\n2) empty() when __isset materialised a falsy value: __get is not called, empty=true:\n";
+$GLOBALS['next_value'] = 0;
+$a = new A;
+var_dump(empty($a->bar));
+
+echo "\n3) empty() with no materialization: __get is still called (legacy path preserved):\n";
+class B {
+    public function __get($n) {
+        echo "  __get($n)\n";
+        return 'value';
+    }
+    public function __isset($n) {
+        echo "  __isset($n)\n";
+        return true;
+    }
+}
+$b = new B;
+var_dump(empty($b->any));
+
+?>
+--EXPECT--
+1) empty() when __isset materialised a truthy value: __get is not called, empty=false:
+  __isset(foo)
+bool(false)
+
+2) empty() when __isset materialised a falsy value: __get is not called, empty=true:
+  __isset(bar)
+bool(true)
+
+3) empty() with no materialization: __get is still called (legacy path preserved):
+  __isset(any)
+  __get(any)
+bool(false)

Zend/tests/magic_methods/gh12695_isset_unchanged.phpt

@@ -0,0 +1,26 @@
+--TEST--
+GH-12695: isset() itself is unchanged (still does not consult __get)
+--FILE--
+<?php
+
+/* The GH-12695 fix only affects `??` and `empty()` (which currently call
+ * __get after __isset). The isset() construct itself has never consulted
+ * __get and continues not to: it returns whatever __isset returned, cast
+ * to bool. */
+
+class C {
+    public function __get($n) {
+        throw new Exception("__get must not be called from a plain isset()");
+    }
+    public function __isset($n) {
+        echo "  __isset($n)\n";
+        return true;
+    }
+}
+$c = new C;
+var_dump(isset($c->any));
+
+?>
+--EXPECT--
+  __isset(any)
+bool(true)

Zend/tests/magic_methods/gh12695_no_materialization.phpt

@@ -0,0 +1,66 @@
+--TEST--
+GH-12695: when __isset() does not materialise the property, __get() is still called
+--FILE--
+<?php
+
+/* The re-check after __isset() must not affect the legacy path when
+ * the property is genuinely magic-only: __get() is still called and
+ * its return value drives `??`'s null check. */
+
+class C {
+    public function __get($n) {
+        echo "  __get($n)\n";
+        return 'from-get';
+    }
+    public function __isset($n) {
+        echo "  __isset($n)\n";
+        return true;
+    }
+}
+
+echo "1) `??` when __isset=true and __get returns a value: __get is called:\n";
+$c = new C;
+var_dump($c->any ?? 'fallback');
+
+echo "\n2) `??` when __isset=true and __get returns null: __get is called and fallback is used:\n";
+class D {
+    public function __get($n) {
+        echo "  __get($n)\n";
+        return null;
+    }
+    public function __isset($n) {
+        echo "  __isset($n)\n";
+        return true;
+    }
+}
+$d = new D;
+var_dump($d->any ?? 'fallback');
+
+echo "\n3) `??` when __isset returns false: __get is not called:\n";
+class E {
+    public function __get($n) {
+        throw new Exception("__get must not be called when __isset returned false");
+    }
+    public function __isset($n) {
+        echo "  __isset($n)\n";
+        return false;
+    }
+}
+$e = new E;
+var_dump($e->any ?? 'fallback');
+
+?>
+--EXPECT--
+1) `??` when __isset=true and __get returns a value: __get is called:
+  __isset(any)
+  __get(any)
+string(8) "from-get"
+
+2) `??` when __isset=true and __get returns null: __get is called and fallback is used:
+  __isset(any)
+  __get(any)
+string(8) "fallback"
+
+3) `??` when __isset returns false: __get is not called:
+  __isset(any)
+string(8) "fallback"

Zend/tests/magic_methods/gh12695_object_released_in_isset.phpt

@@ -0,0 +1,28 @@
+--TEST--
+GH-12695: object freed by __isset() during materialization
+--FILE--
+<?php
+
+/* The re-check after __isset() copies the materialised value into the
+ * caller's return-value buffer before releasing the object. This is
+ * required because __isset() may drop the last external reference to
+ * the object (here via $obj = null), so the property table is freed
+ * by OBJ_RELEASE() right after the re-check. */
+
+class C {
+    public $prop;
+    public function __isset($name) {
+        global $obj;
+        $obj = null;
+        $this->prop = 'materialised';
+        return true;
+    }
+}
+
+$obj = new C();
+unset($obj->prop);
+var_dump($obj->prop ?? 'fb');
+
+?>
+--EXPECT--
+string(12) "materialised"

Zend/zend_object_handlers.c

@@ -934,6 +934,35 @@ ZEND_API zval *zend_std_read_property(zend_object *zobj, zend_string *name, int
 			}
 
 			zval_ptr_dtor(&tmp_result);
+
+			/* __isset() may have materialised the property by writing into
+			 * the property table. Re-check it before deferring to __get(),
+			 * so the freshly-written value is returned directly without a
+			 * redundant __get() call (GH-12695). The value is copied into
+			 * `rv` because the property table can be freed by the OBJ_RELEASE
+			 * below (e.g. when __isset() drops the last external reference
+			 * to the object). */
+			if (IS_VALID_PROPERTY_OFFSET(property_offset)) {
+				retval = OBJ_PROP(zobj, property_offset);
+				if (Z_TYPE_P(retval) != IS_UNDEF) {
+					ZVAL_COPY(rv, retval);
+					retval = rv;
+					OBJ_RELEASE(zobj);
+					goto exit;
+				}
+			} else if (IS_DYNAMIC_PROPERTY_OFFSET(property_offset)) {
+				if (zobj->properties != NULL) {
+					retval = zend_hash_find(zobj->properties, name);
+					if (retval) {
+						ZVAL_COPY(rv, retval);
+						retval = rv;
+						OBJ_RELEASE(zobj);
+						goto exit;
+					}
+				}
+			}
+			retval = &EG(uninitialized_zval);
+
 			if (zobj->ce->__get && !((*guard) & IN_GET)) {
 				goto call_getter;
 			}
@@ -2487,7 +2516,20 @@ ZEND_API int zend_std_has_property(zend_object *zobj, zend_string *name, int has
 			result = zend_is_true(&rv);
 			zval_ptr_dtor(&rv);
 			if (has_set_exists == ZEND_PROPERTY_NOT_EMPTY && result) {
-				if (EXPECTED(!EG(exception)) && zobj->ce->__get && !((*guard) & IN_GET)) {
+				/* GH-12695, see above. */
+				zval *prop = NULL;
+				if (IS_VALID_PROPERTY_OFFSET(property_offset)) {
+					prop = OBJ_PROP(zobj, property_offset);
+					if (Z_TYPE_P(prop) == IS_UNDEF) {
+						prop = NULL;
+					}
+				} else if (IS_DYNAMIC_PROPERTY_OFFSET(property_offset)
+						&& zobj->properties != NULL) {
+					prop = zend_hash_find(zobj->properties, name);
+				}
+				if (prop) {
+					result = i_zend_is_true(prop);
+				} else if (EXPECTED(!EG(exception)) && zobj->ce->__get && !((*guard) & IN_GET)) {
 					(*guard) |= IN_GET;
 					zend_std_call_getter(zobj, name, &rv);
 					(*guard) &= ~IN_GET;