Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I was hunting down a bug earlier today and ended up in ContentEntityBase::__isset() and noticed that is contained an else
clause that was not needed.
I checked further in the file and noticed a couple of other places where that was also unneeded.
- Remove unneeded nesting \Drupal\Core\Entity\ContentEntityBase::__clone()
- Remove unneeded else \Drupal\Core\Entity\ContentEntityBase::__isset()
- Remove unneeded else \Drupal\Core\Entity\ContentEntityBase::__unset
Comment | File | Size | Author |
---|---|---|---|
#30 | interdiff-2854926-27-30.txt | 626 bytes | himanshu-dixit |
#30 | 2854926-30.patch | 6.39 KB | himanshu-dixit |
#27 | interdiff-06-27.txt | 1.45 KB | himanshu-dixit |
#27 | 2854926-27.patch | 6.75 KB | himanshu-dixit |
#23 | 2854926-23.patch | 6.62 KB | himanshu-dixit |
Comments
Comment #2
borisson_This only a small optimisation and thus I marked this as minor.
Comment #3
boaloysius CreditAttribution: boaloysius as a volunteer commentedManually checked the code.
Correctly removed unneeded nesting, without disturbing the flow of the code.
Comment #4
boaloysius CreditAttribution: boaloysius as a volunteer commentedComment #5
boaloysius CreditAttribution: boaloysius as a volunteer commentedThese screenshots are self-explanatory.
Comment #6
xjmThis is a nice cleanup, thanks!
I did a
git diff -w
on this to verify the changes were correct:Removes an
else
condition that will not be reached if theif
was ever checked.Adds an early return to follow the same pattern as the first hunk.
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.
Comment #9
xjmSome issue with test result reporting; queueing them again.
Comment #13
xjmTestbot is experiencing technical difficulties; waiting for that to resolve before retesting again.
Comment #14
xjmSo, 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.
Comment #15
xjmComment #18
xjmPosted #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!
Comment #21
xjm@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!Comment #22
xjm@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.Comment #23
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer commentedSorry, i haven't attached the interdiff right now. I would upload it soon
Comment #25
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer commentedComment #26
alexpottThis
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
Comment #27
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer commentedI 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.
Comment #28
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer commentedComment #29
alexpottI think this change is wrong. There's no need to change the __unset at all the early return makes the code harder to understand.
Comment #30
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer commentedOk, Here is the new patch.
Comment #31
alexpottThis 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.
Comment #34
xjmThis 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!
Comment #36
himanshu-dixit CreditAttribution: himanshu-dixit as a volunteer and at Google Summer of Code commentedAttributing the contribution to GSoC.
Comment #37
boaloysius CreditAttribution: boaloysius as a volunteer and at Google Summer of Code commented