Problem/Motivation

According to the documentation of \Drupal\Core\Entity\EntityTypeInterface::getHandlerClass() the method should return NULL if a handler class is not set.

We never test for this outcome.

Proposed resolution

Add a test case to Drupal\Tests\Core\Entity\EntityTypeTest to ensure that a NULL result is possible.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#18 interdiff-15-18.txt524 bytesHardik_Patel_12
#18 2910775-18.patch658 bytesHardik_Patel_12
#15 2910775-15.patch1.12 KBSivaji_Ganesh_Jojodae
#13 2910775-13.patch1.43 KBSivaji_Ganesh_Jojodae
#2 2910775-2.patch514 byteshchonov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hchonov created an issue. See original summary.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
FileSize
2910775-2.patch514 bytes
Sam152’s picture

Sam152 CreditAttribution: Sam152 as a volunteer and at PreviousNext commented

If a function doesn't return anything it behaves exactly the same as if it returned NULL. See below:

php > $foo = function() { };
php > var_export($foo());
NULL
php > $bar = function() { return NULL; };
php > var_export($foo() === $bar());
true
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Category: Bug report » Task

Yeah this is not a bug , maybe a task ... slightly better code?

vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer and at TATA Consultancy Services, Solathat for Pfizer, Inc. commented
Issue tags: +Novice, +Vienna2017
vijaycs85’s picture

vijaycs85
Location London, UK
CreditAttribution: vijaycs85 as a volunteer and at TATA Consultancy Services, Solathat for Pfizer, Inc. commented
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: EntityType::getHandlerClass will not return NULL if a handler class is not set, but according to the interface it should » Add test case for EntityType::getHandlerClass returning NULL
Issue summary: View changes
Status: Needs review » Needs work

Rescoping because EntityType::getHandlerClass() seems to return NULL by not returning anything based on hasHandlerClass().

This makes it an easy unit test to add to Drupal\Tests\Core\Entity\EntityTypeTest, because we never test for this outcome.

Ivan Berezhnov’s picture

Ivan Berezhnov CreditAttribution: Ivan Berezhnov as a volunteer and at Drupal Ukraine Community for Levi9 commented
Issue tags: +CSKyiv18

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.7.x-dev

Drupal 8.6.0-alpha1 will be released the week of July 16, 2018, which means new developments and disruptive changes should now be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.7.x-dev » 8.8.x-dev

Drupal 8.7.0-alpha1 will be released the week of March 11, 2019, which means new developments and disruptive changes should now be targeted against the 8.8.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Sivaji_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae as a volunteer and commented
Status: Needs work » Needs review
FileSize
2910775-13.patch1.43 KB

Not sure if the attached test & code change is what intended. Please review.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityTypeTest.php
@@ -169,6 +169,23 @@ public function testGetHandler() {
+  public function testGetHandlerClassNull() {
...
+    $this->assertNull($entity_type->getHandlerClass('foo'));
+    $this->assertNull($entity_type->getHandlerClass('foo', 'bar'));

Instead of creating a new test we could incorporate these 2 lines in ::testGetHandler().

Sivaji_Ganesh_Jojodae’s picture

Sivaji_Ganesh_Jojodae CreditAttribution: Sivaji_Ganesh_Jojodae as a volunteer and commented
Status: Needs work » Needs review
FileSize
2910775-15.patch1.12 KB

Makes sense. Patch re-rolled to reflect the change.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented
Status: Needs review » Reviewed & tested by the community

Thank you.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/lib/Drupal/Core/Entity/EntityType.php
@@ -465,6 +465,7 @@ public function getHandlerClass($handler_type, $nested = FALSE) {
+    return NULL;

This is not needed. It is implicit when there is no return. I do not want to encourage a spate of issues adding return NULL everywhere.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commented
FileSize
2910775-18.patch658 bytes
interdiff-15-18.txt524 bytes

function test()
{
return null;
}

function test()
{
return;
}

function test()
{

}

In all cases there for var_dump(test()); will be:

NULL

Kindly review a new patch.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commented
Status: Needs work » Needs review
ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Needs review » Reviewed & tested by the community

Patch #18 looks good to me, so making it RTBC.

hchonov’s picture

hchonov
Primary language Bulgarian
Location 🇪🇺🇩🇪🇧🇬
CreditAttribution: hchonov at bio.logis Genetic Information Management GmbH commented

This is not needed. It is implicit when there is no return. I do not want to encourage a spate of issues adding return NULL everywhere.

I understand your worries, however I consider no explicit return a bad practice and I hope that there aren't that many places without explicit returns.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

@hchonov okay I committed #15. I can buy the argument that when you have a return it's best that all returns are explicit.

Committed and pushed b6bf07b8aa to 9.0.x and a1468fef85 to 8.9.x. Thanks!

  • alexpott committed b6bf07b on 9.0.x 
    Issue #2910775 by Sivaji, Hardik_Patel_12, hchonov, Mile23: Add test...

  • alexpott committed a1468fe on 8.9.x 
    Issue #2910775 by Sivaji, Hardik_Patel_12, hchonov, Mile23: Add test...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.