Problem/Motivation

When using the \Drupal\field\Entity\FieldStorageConfig::loadByName() method, the returned results don't match the return type given in the DocBlock comment. This has lead to us having an uncaught exception that we didn't expect.

Proposed resolution

Fix up the return type documented in the DocBlock to improve the DX and help catch problems before they happen.

Remaining tasks

None.

CommentFileSizeAuthor
#16 incorrect_return_type-2918290-16.patch1.77 KBsomersoft
#14 incorrect_return_type-2918290-14.patch1.22 KBsomersoft
#4 incorrect_return_type-2918290-4.patch589 byteschOP
#2 incorrect_return_type-2918290-2.patch583 byteschOP
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chOP created an issue. See original summary.

chOP’s picture

chOP CreditAttribution: chOP at Deloitte Digital commented
FileSize
incorrect_return_type-2918290-2.patch583 bytes

I've corrected return types for this method. Please review.

The return type here should be consistent with \Drupal\Core\Entity\EntityStorageInterface::load() as we're simply returning a result from that interface implementation.

chOP’s picture

chOP CreditAttribution: chOP at Deloitte Digital commented
Status: Active » Needs review
chOP’s picture

chOP CreditAttribution: chOP at Deloitte Digital commented
FileSize
incorrect_return_type-2918290-4.patch589 bytes

Okay, I've reviewed the return type and adjusted it to more accurately reflect the actual interface that can be expected.

The method will return \Drupal\field\FieldStorageConfigInterface or null. The FieldStorageConfigInterface is an extended implementation of the EntityInterface.

Please review this updated patch.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs review » Needs work 
+++ b/core/modules/field/src/Entity/FieldStorageConfig.php
@@ -807,7 +807,7 @@ protected function getFieldItemClass() {
    *   otherwise NULL.

The current coding standards say that this could be simply @return $this without any description :)

chOP’s picture

chOP CreditAttribution: chOP at Deloitte Digital commented

@amateescu:
But that is not what it returns. It returns an object, or null, by loading from another Class's static method. I could understand putting @return $this if that's what the method did, but it doesn't. This is a static method, so it cannot return $this. Nor does it ever return itself, the static class.

If you said @return $this you'd be expecting that this method returned a reference to the instance of itself. It doesn't do that. It returns a reference to an entirely different object instance.

amateescu’s picture

amateescu CreditAttribution: amateescu for Pfizer, Inc. commented
Status: Needs work » Needs review

@chOP, right, you have a very good point :)

Would you be ok with fixing the return documentation for \Drupal\field\Entity\FieldConfig::loadByName() and \Drupal\Core\Field\Entity\BaseFieldOverride::loadByName in this issue?

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.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
CreditAttribution: borisson_ as a volunteer commented
Status: Needs review » Needs work

#7 needs to be fixed before we can get this in.

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.

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

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

somersoft’s picture

somersoft CreditAttribution: somersoft commented
Status: Needs work » Needs review
FileSize
incorrect_return_type-2918290-14.patch1.22 KB

Add the reqest change in #7 .
These changes help with removing false errors when running the Drupal check program.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Needs work

#14 covers \Drupal\field\Entity\FieldStorageConfig::loadByName() but not \Drupal\field\Entity\FieldConfig::loadByName() as requested in #7.

somersoft’s picture

somersoft CreditAttribution: somersoft commented
Status: Needs work » Needs review
FileSize
incorrect_return_type-2918290-16.patch1.77 KB

Sorry, I have now added the requested document change.
In my code base the Base Field Override is called and hence retaining that change as well.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Thanks, this looks good now.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 16: incorrect_return_type-2918290-16.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Reviewed & tested by the community

QuickEditIntegrationTest strikes again.

  • catch committed 83fcced on 9.1.x 
    Issue #2918290 by chOP, somersoft, longwave, amateescu: Incorrect return...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed 83fcced and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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