Following on from #3203611: Wrong typehint in \Drupal\field\Entity\FieldConfig::loadByName, the documented return type of \Drupal\field\Entity\FieldConfig::loadByName is still not exactly correct.
The return type should be fully qualified, to not cause tooling like PHPStan to blow up, and PHP Storm to report errors.
Change the return doc to
@return static|null
| Comment | File | Size | Author |
|---|---|---|---|
| #31 | 3251856-31.patch | 579 bytes | shubham chandra |
| #26 | 3251856-26.patch | 579 bytes | elber |
| #20 | 3251856-20.patch | 557 bytes | elber |
| #15 | 3251856-15.patch | 552 bytes | elber |
| #11 | interdiff_7-11.txt | 89 KB | elber |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #3
dpiComment #4
jibranThank you for your contribution. This is a huge improvement for the Drupal documentation.
Comment #5
dpiThank you I appreciate that you appreciate me smiley face
Comment #6
navneet0693 commentedI think the correct namespace is
\Drupal\Core\Field\FieldConfigInterfaceComment #7
navneet0693 commentedComment #8
dpiMaybe this should be
@return static, since this class should implement both interfaces, but neither interface extend eachother.An intersection type is also possible, but ugly.
Comment #9
dpiComment #10
elberComment #11
elberComment #12
dpiPatch contains changes unrelated to the issue at hand.
Comment #13
elberComment #14
elberSorry can you explain me what's your desire? I don't understand.
Comment #15
elberComment #16
beatrizrodriguesSeems ok to me, the problem from the previous patch was fixed, and the suggestion that was given at #8 was followed. Patch applied perfectly.
Comment #17
alexpottI've tried to understand we what #8 means by
I think the patch in #7 is correct. If
staticreally is correct then it needs to bestatic|null.Comment #18
dpi@AlexPott
\Drupal\field\Entity\FieldConfigimplements both\Drupal\Core\Field\FieldConfigInterfaceand\Drupal\field\FieldConfigInterface. But both of theseFieldConfigInterfacedont implement eachother. So it wouldn't be ideal to typehint either interface since you'd miss out on methods.staticworks since its a static method that will always be returning an instance of self. Short of doing something like\Drupal\Core\Field\FieldConfigInterface&\Drupal\field\FieldConfigInterface, introducing a new interface, or updating either interface to implement anotherUltimately
static|nullis what we're aiming for. Not sure why #15 removed the null part.Comment #19
elberComment #20
elberComment #21
hmendes commentedThe last patch addresses the comment
Ultimately static|null is what we're aiming for. Not sure why #15 removed the null part.and applies perfectly.Changing it to RTBC.
Comment #22
alexpott@dpi I guess the question I have is why does
Drupal\field\FieldConfigInterfacenot implementDrupal\Core\Field\FieldConfigInterface. I think it should. It seems an oversight. If anyone has done alternate configurable field implementation is not going to only implementDrupal\field\FieldConfigInterfacebecause it'd need to implementDrupal\Core\Field\FieldConfigInterfacetoo - so let's fix this. By addingDrupal\Core\Field\FieldConfigInterfacetoDrupal\field\FieldConfigInterface.This would have the benefit of improving all of the typehints listed on http://grep.xnddx.ru/search?text=Drupal%5Cfield%5CFieldConfigInterface&f...
I guess this happened because of how BaseFieldOverride was carved off from configurable fields during D8 development.
Comment #23
elberComment #24
dpi@alexpott we have #2818877: field's FieldConfigInterface should extends Core's FieldConfigInterface and #2399301: There should be only one FieldConfigInterface already, which cover history and resolutions.
Even if either interface issue is resolved
static|nullshould still be correct?Comment #25
alexpottImo this is a dupe of #2818877: field's FieldConfigInterface should extends Core's FieldConfigInterface ... once that is done then the docs for the method are correct - because this calls out to
\Drupal::entityTypeManager()->getStorage('field_config')->load()you actually have no guarantee that someone has not swapped out the class for something that does not extend from FieldConfig. However the class that is return should definitely implement both of the FieldConfig interfaces.Comment #26
elberHello @alexpott I'm sending my patch. But I don't know if I understand what is your objective. Can you explain me again?
Comment #28
andregp commentedI guess @alexpott point on #25 is right. If #2818877: field's FieldConfigInterface should extends Core's FieldConfigInterface is fixed then the type hint will be correct and the change from MR#1458 will be all we need here.
Comment #29
anjali rathodJust checked , the changes mentioned above are present in the code base as the result of MR#1458 .
Comment #30
dpi@29 this doesn't seem correct resolution
Comment #31
shubham chandra commentedAdded patch against #26 in Drupal 10.1.x
Comment #32
dpi28 and 29 suggest all we need is the MR changes, which is the leading slash change.
26 and 31 still dont actually fix the leading slash, differing from the MR by removing
nullreturn, which doesnt seem right.Comment #33
kingdutchComment #34
acbramley commentedAs of #3322743: Fix PHPStan L2 errors "Parameter $foo of method Foo::bar() has invalid type Foo\Baz." and "Method Foo::bar() has invalid return type Foo\Baz. " (committed a week ago) - the missing leading slash has been added which means the original problem of phpstan complaining about invalid types is now fixed.
Should we close this as outdated?
Comment #36
acbramley commentedAs per above, closing.