Repository: getsentry/sentry-php (php)

Issue:
Do not call magic methods when seralizing object IDs in stack traces
This also invokes magic `__isset()` (and then `__get()` ) method for objects, which can be problematic if these magic methods have side effects. Which is exactly what I had to investigate and deal with in our own codebase today.

I'd propose that this behavior (magically getting some kind of ID of objects present within traceback) should check if the property _actually_ exists first _(via `property_exists()`, which doesn't invoke magic methods)_.

Or at least this magic feature should be optional - i,e. there should be an option to disable it.

_Originally posted by @smuuf in https://github.com/getsentry/sentry-php/pull/1443#discussion_r1116010136_
            

Pull request:
Prevent calling magic methods to retrieve ID from object
It seems that in some codebases this causes side-effects, it might be better to be a little more strict/safe here.

Fixes #1476.

Patch:
diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon
index f96d985..c3ddeb1 100644
--- a/phpstan-baseline.neon
+++ b/phpstan-baseline.neon
@@ -20,11 +20,6 @@ parameters:
 			count: 1
 			path: src/Dsn.php
 
-		-
-			message: "#^Offset 'host'\\|'path'\\|'scheme'\\|'user' on array\\{scheme\\?\\: string, host\\?\\: string, port\\?\\: int\\<0, 65535\\>, user\\?\\: string, pass\\?\\: string, path\\?\\: string, query\\?\\: string, fragment\\?\\: string\\} in isset\\(\\) always exists and is not nullable\\.$#"
-			count: 1
-			path: src/Dsn.php
-
 		-
 			message: "#^Offset 'path' does not exist on array\\{scheme\\: 'http'\\|'https', host\\?\\: string, port\\?\\: int\\<0, 65535\\>, user\\?\\: string, pass\\?\\: string, path\\?\\: string, query\\?\\: string, fragment\\?\\: string\\}\\.$#"
 			count: 4
@@ -40,11 +35,6 @@ parameters:
 			count: 1
 			path: src/Dsn.php
 
-		-
-			message: "#^Result of && is always false\\.$#"
-			count: 3
-			path: src/EventHint.php
-
 		-
 			message: "#^Access to constant CONNECT_TIMEOUT on an unknown class GuzzleHttp\\\\RequestOptions\\.$#"
 			count: 1
diff --git a/src/Serializer/AbstractSerializer.php b/src/Serializer/AbstractSerializer.php
index b1f6f66..510a504 100644
--- a/src/Serializer/AbstractSerializer.php
+++ b/src/Serializer/AbstractSerializer.php
@@ -241,14 +241,20 @@ protected function serializeValue($value)
         }
 
         if (\is_object($value)) {
+            $reflection = new \ReflectionObject($value);
+
             $objectId = null;
-            if (isset($value->id)) {
-                $objectId = $value->id;
-            } elseif (\is_callable([$value, 'getId']) && method_exists($value, 'getId')) {
-                $objectId = $value->getId();
+            if ($reflection->hasProperty('id') && ($idProperty = $reflection->getProperty('id'))->isPublic()) {
+                $objectId = $idProperty->getValue($value);
+            } elseif ($reflection->hasMethod('getId') && ($getIdMethod = $reflection->getMethod('getId'))->isPublic()) {
+                try {
+                    $objectId = $getIdMethod->invoke($value);
+                } catch (\Throwable $e) {
+                    // Do nothing on purpose
+                }
             }
 
-            return 'Object ' . \get_class($value) . ((null !== $objectId) ? '(#' . $objectId . ')' : '');
+            return 'Object ' . $reflection->getName() . ((null !== $objectId) ? '(#' . $objectId . ')' : '');
         }
 
         if (\is_resource($value)) {


Test patch:
diff --git a/tests/Serializer/AbstractSerializerTest.php b/tests/Serializer/AbstractSerializerTest.php
index 2a3b447..a1a14f9 100644
--- a/tests/Serializer/AbstractSerializerTest.php
+++ b/tests/Serializer/AbstractSerializerTest.php
@@ -63,6 +63,15 @@ public function testObjectsWithIdPropertyAreStrings(): void
         $this->assertSame('Object Sentry\Tests\Serializer\SerializerTestObjectWithIdProperty(#bar)', $result);
     }
 
+    public function testObjectsWithMagicIdPropertyDoesNotInvokeMagicMethods(): void
+    {
+        $serializer = $this->createSerializer();
+        $input = new SerializerTestObjectWithMagicGetAndIssetMethods();
+        $result = $this->invokeSerialization($serializer, $input);
+
+        $this->assertSame('Object Sentry\Tests\Serializer\SerializerTestObjectWithMagicGetAndIssetMethods', $result);
+    }
+
     public function testObjectsWithSerializerTestObjectWithGetIdMethodAreStrings(): void
     {
         $serializer = $this->createSerializer();
@@ -609,6 +618,30 @@ public function getId()
     }
 }
 
+class SerializerTestObjectWithMagicGetAndIssetMethods
+{
+    public function __isset(string $name): bool
+    {
+        throw new \RuntimeException('We should not reach this!');
+    }
+
+    /**
+     * @return mixed
+     */
+    public function __get(string $name)
+    {
+        throw new \RuntimeException('We should not reach this!');
+    }
+
+    /**
+     * @param mixed $value
+     */
+    public function __set(string $name, $value): void
+    {
+        throw new \RuntimeException('We should not reach this!');
+    }
+}
+
 class Invokable
 {
     public function __invoke(): bool