Problem/Motivation

This is a followup to #2341357: Views entity area config is not deployable and missing dependencies, where a new method (EntityManagerInterface::loadEntityByConfigTarget()) was introduced with a return value that I believe is incorrect.

Proposed resolution

Fix it.

Remaining tasks

Review & commit.

User interface changes

Nope.

API changes

No.

Beta eval: This is an unfrozen change: API docs bug fix.

CommentFileSizeAuthor
#8 interdiff.txt2.37 KBamateescu
#8 2417339-8.patch4.12 KBamateescu
#5 intediff.txt1.93 KBamateescu
#5 2417339-5.patch2.3 KBamateescu
#3 return_doc_entitymanagerinterface-2417339-3.patch1.44 KBshashikant_chauhan
#1 2417339.patch1.45 KBamateescu

Comments

amateescu’s picture

amateescu commented
Status: Active » Needs review
StatusFileSize
new 2417339.patch1.45 KB

For example, Drupal\Core\Entity\Entity::load() does not mention |null in the @return doc, so I think loadEntityByConfigTarget() and loadEntityByUuid() should do the same.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

I'm quite convinced that this is not the right thing to do, adding |null gives you additional information

$ ag "\|null" | wc -l
     798
shashikant_chauhan’s picture

shashikant_chauhan commented
StatusFileSize
new return_doc_entitymanagerinterface-2417339-3.patch1.44 KB

Added 'null' and recreated patch based on #1

shashikant_chauhan’s picture

shashikant_chauhan commented
Status: Needs work » Needs review
amateescu’s picture

amateescu commented
StatusFileSize
new 2417339-5.patch2.3 KB
new intediff.txt1.93 KB

The initial patch also fixed the UUID part in the doc, which actually had to reference 'target'. Let's also fix the documentation for Drupal\Core\Entity\Entity::load() and loadMultiple() while we're there.

Interdiff is to #1.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

One nitpick:

    * @return static[]
-   *   An array of entity objects indexed by their IDs.
+   *   An array of entity objects indexed by their IDs. Returns an empty array
+   *   if no matching entities found.
    */
   public static function loadMultiple(array $ids = NULL) {

==> ... if no matching entities are found.

The other changes look good, if everyone agrees they are factually correct.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs review » Needs work
amateescu’s picture

amateescu commented
Status: Needs work » Needs review
StatusFileSize
new 2417339-8.patch4.12 KB
new interdiff.txt2.37 KB

The string from #6 also exists in a bunch of other places so I updated all of them for consistency.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

A bit out of scope for this issue, but a good fix.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Documentation is not frozen in beta. Committed 097b83f and pushed to 8.0.x. Thanks!

  • alexpott committed 097b83f on 8.0.x 
    Issue #2417339 by amateescu, shashikant_chauhan: Fix @return...

Status: Fixed » Closed (fixed)

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