Merge pull request #3 from ChessCom/php-isset-fix

[php][ext] - Fix isset behaviour

Author: FabioBatSilva

php/ext/google/protobuf/message.c

@@ -85,11 +85,11 @@ static void Message_dtor(zend_object* obj) {
 }
 
 /**
- * get_field()
+ * lookup_field()
  *
  * Helper function to look up a field given a member name (as a string).
  */
-static const upb_FieldDef* get_field(Message* msg, zend_string* member) {
+static const upb_FieldDef* lookup_field(Message* msg, zend_string* member) {
   if (!msg || !msg->desc || !msg->desc->msgdef) {
     zend_throw_exception_ex(NULL, 0,
                             "Couldn't find descriptor. "
@@ -104,8 +104,26 @@ static const upb_FieldDef* get_field(Message* msg, zend_string* member) {
       m, ZSTR_VAL(member), ZSTR_LEN(member));
 
   if (!f) {
-    zend_throw_exception_ex(NULL, 0, "No such property %s.",
-                            ZSTR_VAL(msg->desc->class_entry->name));
+    return NULL;
+  }
+
+  return f;
+}
+
+/**
+ * get_field()
+ *
+ * Helper function to get up a field given a member name (as a string).
+ * if the field is not found, a PHP warning is emitted and NULL is returned.
+ */
+static const upb_FieldDef* get_field(Message* msg, zend_string* member) {
+  const upb_FieldDef* f = lookup_field(msg, member);
+
+  if (!f && msg && msg->desc) {
+    // if the descriptor is undefined, an exception was already thrown.
+    php_error(E_WARNING, "Undefined property: %s::$%s",
+                            ZSTR_VAL(msg->desc->class_entry->name),
+                            ZSTR_VAL(member));
   }
 
   return f;
@@ -250,15 +268,11 @@ static int Message_compare_objects(zval* m1, zval* m2) {
 static int Message_has_property(zend_object* obj, zend_string* member,
                                 int has_set_exists, void** cache_slot) {
   Message* intern = (Message*)obj;
-  const upb_FieldDef* f = get_field(intern, member);
+  const upb_FieldDef* f = lookup_field(intern, member);
 
   if (!f) return 0;
 
-  if (!upb_FieldDef_HasPresence(f)) {
-    zend_throw_exception_ex(
-        NULL, 0,
-        "Cannot call isset() on field %s which does not have presence.",
-        upb_FieldDef_Name(f));
+  if (upb_FieldDef_IsOptional(f) && !upb_FieldDef_HasPresence(f)) {
     return 0;
   }
 
@@ -284,7 +298,7 @@ static int Message_has_property(zend_object* obj, zend_string* member,
 static void Message_unset_property(zend_object* obj, zend_string* member,
                                    void** cache_slot) {
   Message* intern = (Message*)obj;
-  const upb_FieldDef* f = get_field(intern, member);
+  const upb_FieldDef* f = lookup_field(intern, member);
 
   if (!f) return;
 

php/tests/GeneratedClassTest.php

@@ -56,6 +56,31 @@ public function testSetterGetter()
         $this->assertSame(1, $m->getOptionalInt32());
     }
 
+    #########################################################
+    # Test isset / property_exists
+    #########################################################
+
+    public function testIsset()
+    {
+        $message = new TestMessage();
+
+        $this->assertTrue(property_exists(TestMessage::class, 'optional_string'));
+        $this->assertTrue(property_exists($message, 'optional_string'));
+        $this->assertFalse(isset($message->optional_string));
+
+        $message->setOptionalString('bar');
+
+        $this->assertTrue(property_exists(TestMessage::class, 'optional_string'));
+        $this->assertFalse(property_exists(TestMessage::class, 'foo'));
+
+        $this->assertTrue(property_exists($message, 'optional_string'));
+        $this->assertFalse(property_exists($message, 'foo'));
+
+        // isset() should return false for attributes that are not public.
+        $this->assertFalse(isset($message->optional_string));
+        $this->assertFalse(isset($message->foo));
+    }
+
     #########################################################
     # Test int32 field.
     #########################################################

src/google/protobuf/repeated_ptr_field_unittest.cc

@@ -559,7 +559,7 @@ TEST(RepeatedPtrFieldTest, AddAllocated) {
   EXPECT_EQ(moo, &field.Get(index));
 }
 
-TEST(RepeatedPtrFieldTest, AddMethodsDontAcceptNull) {
+TEST(RepeatedPtrFieldDeathTest, AddMethodsDontAcceptNull) {
 #if !defined(NDEBUG)
   RepeatedPtrField<std::string> field;
   EXPECT_DEATH(field.AddAllocated(nullptr), "nullptr");
@@ -1112,7 +1112,7 @@ TEST(RepeatedPtrFieldTest, Cleanups) {
 }
 
 
-TEST(RepeatedPtrFieldTest, CheckedGetOrAbortTest) {
+TEST(RepeatedPtrFieldDeathTest, CheckedGetOrAbortTest) {
   RepeatedPtrField<std::string> field;
 
   // Empty container tests.
@@ -1127,7 +1127,7 @@ TEST(RepeatedPtrFieldTest, CheckedGetOrAbortTest) {
   EXPECT_DEATH(internal::CheckedGetOrAbort(field, -1), "index: -1, size: 2");
 }
 
-TEST(RepeatedPtrFieldTest, CheckedMutableOrAbortTest) {
+TEST(RepeatedPtrFieldDeathTest, CheckedMutableOrAbortTest) {
   RepeatedPtrField<std::string> field;
 
   // Empty container tests.