Remove unneeded control structures in ContentEntityBase

Manually checked the code.
Correctly removed unneeded nesting, without disturbing the flow of the code.

These screenshots are self-explanatory.

This is a nice cleanup, thanks!

I did a git diff -w on this to verify the changes were correct:

1. diff --git a/core/lib/Drupal/Core/Entity/ContentEntityBase.php b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   index 5cd27c7c61..78982a8d9e 100644
   --- a/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
   @@ -1014,11 +1014,10 @@ public function __isset($name) {
        if ($this->hasField($name)) {
          return TRUE;
        }
   +
        // For non-field properties, check the internal values.
   -    else {
        return isset($this->values[$name]);
      }
   -  }
    
      /**
       * Implements the magic method for unset().
   

   Removes an else condition that will not be reached if the if was ever checked.

2. @@ -1027,12 +1026,12 @@ public function __unset($name) {
        // Unsetting a field means emptying it.
        if ($this->hasField($name)) {
          $this->get($name)->setValue(array());
   +      return TRUE;
        }
   +
        // For non-field properties, unset the internal value.
   -    else {
        unset($this->values[$name]);
      }
   -  }
   

   Adds an early return to follow the same pattern as the first hunk.

3.    /**
       * {@inheritdoc}
   @@ -1067,7 +1066,10 @@ public function createDuplicate() {
      public function __clone() {
        // Avoid deep-cloning when we are initializing a translation object, since
        // it will represent the same entity, only with a different active language.
   -    if (!$this->translationInitialize) {
   +    if ($this->translationInitialize) {
   +      return TRUE;
   +    }
   +
        // The translation is a different object, and needs its own TypedData
        // adapter object.
        $this->typedData = NULL;
   @@ -1123,7 +1125,6 @@ public function __clone() {
          }
        }
      }
   -  }
    
      /**
       * {@inheritdoc}

   Inverts the earlier check to also use the return early pattern and therefore avoid an entire level of nesting.

Whenever we do refactoring like this, it's good to confirm that there's test coverage so we know the refactoring is safe. I've attached three test-only patches that should each fail if each hunk has test coverage, plus re-attached the actual patch. Leaving RTBC since I'm not actually making any changes, just testing for coverage.

Some issue with test result reporting; queueing them again.

Testbot is experiencing technical difficulties; waiting for that to resolve before retesting again.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1027,11 +1026,11 @@ public function __unset($name) {
     // Unsetting a field means emptying it.
     if ($this->hasField($name)) {
       $this->get($name)->setValue(array());
+      return TRUE;
     }
+
     // For non-field properties, unset the internal value.
-    else {
-      unset($this->values[$name]);
-    }
+    unset($this->values[$name]);

So, it looks like this hunk does not have test coverage. The other two do, as 1 failed clearly and 3, uh, broke testbot.

That said, all three are magic methods.

Posted #2855084: Improve unit test coverage for entity magic methods and committed to 8.4.x and 8.3.x, especially since the uncovered case was the easiest to read anyway. Thanks!

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1067,60 +1066,62 @@ public function createDuplicate() {
+    if ($this->translationInitialize) {
+      return TRUE;
+    }

@alexpott pointed out returning TRUE is weird and return; would be better. Reverted to look more closely at the return values and whether this approach is even valid for the magic methods. Thanks!

@alexpott also pointed out that the second patch that didn't fail wasn't even testing what I meant to test, so that's irrelevant, and not worth accidentally breaking the bot for implicit coverage to look for explicit coverage. It's in core/tests/Drupal/KernelTests/Core/Entity/EntityFieldTest.php which I would have seen if I just looked instead of abusing testbot incorrectly. Sorry for the distraction. NW for #21 though still.

Sorry, i haven't attached the interdiff right now. I would upload it soon

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1027,11 +1026,11 @@ public function __unset($name) {
+      return TRUE;

This return TRUE; is actually the same as __clone() - unset returns a void too. In fact I think these changes to __unset() are unnecessary and make the developer think more. I guess we might have been trying to keep the method inline with __isset() but that seems wrong.

The changes to __clone are good - removing a level of nesting there helps developers. The other changes I'm not that sure about. If we want to change the __isset() maybe we should consider a ternary... like so

  /**
   * Implements the magic method for isset().
   */
  public function __isset($name) {
    // "Official" Field API fields are always set. For non-field properties,
    // check the internal values.
    return $this->hasField($name) ? TRUE : isset($this->values[$name]);
  }

I am uploading the new patch as per the suggestions mentioned in #26. Also don't you think the issue should go to 8.4.x-dev.

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1027,11 +1022,11 @@ public function __unset($name) {
     // Unsetting a field means emptying it.
     if ($this->hasField($name)) {
       $this->get($name)->setValue(array());
+      return;
     }
+
     // For non-field properties, unset the internal value.
-    else {
-      unset($this->values[$name]);
-    }
+    unset($this->values[$name]);

I think this change is wrong. There's no need to change the __unset at all the early return makes the code harder to understand.

Ok, Here is the new patch.

This looks good. I've done a final git diff --color-words and can confirm that the only logic change to __clone is the early return after inverting the logic to not deep clone when we are initializing a translation object.

Thanks for addressing my feedback @himanshu-dixit.

This looks good to me to. The ternary is much more readable. Thanks @alexpott for catching the issues with this. Recommitted the latest patch to both branches to avoid unnecessary merge conflicts, since we're in beta and we were careful to review the code flow. Thanks!

Attributing the contribution to GSoC.