Quite simply, with all the new work in Entity storage controllers, entity_load() now accepts a 'mixed' argument rather than an integer, and the docs are not up to date.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xtfer’s picture

Status: Active » Needs review
FileSize
11.61 KB

Patch!

xtfer’s picture

Status: Needs review » Needs work

Wrong patch.

xtfer’s picture

xtfer’s picture

Status: Needs work » Needs review
larowlan’s picture

Component: entity system » documentation
Status: Needs review » Reviewed & tested by the community

unless bot disagrees, config entities have (string) machine names for ids, so this is confusing

tim.plunkett’s picture

Why not string|int then? When I see mixed I generally think stdClass...

larowlan’s picture

Status: Reviewed & tested by the community » Needs work

yeah fair point

xtfer’s picture

Status: Needs work » Needs review

If thats the case, then ALL the Entity Storage classes need to be changed as well, however I cant see a reason why you couldnt pass a class as an identifier to an extended storage controller, which knows how to extract it.

So, 'mixed' is probably as specific as we can actually get, even though 99.9% of cases are probably strings.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community
larowlan’s picture

Thats a link to EntityStorageControllerInterface::load

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs work

Um. What can you actually pass in here? Saying it is "mixed" doesn't tell me anything at all.

Also, "id" is a psychological term. Can you change the next line to read ID or better yet "identifier"? Or better yet, explain what the possible values are or what field it corresponds to on some class?

xtfer’s picture

Um. What can you actually pass in here? Saying it is "mixed" doesn't tell me anything at all.

Thats an issue with the EntityStorageControllerInterface, since it will (in fact) take any argument.

Also, "id" is a psychological term. Can you change the next line to read ID or better yet "identifier"? Or better yet, explain what the possible values are or what field it corresponds to on some class?

Wow. Are you trolling?

I believe entities are too broad in their implementation for us to say what "field" it corresponds to, and we certainly can't specify possibly values. Again, this is really down to the way entities are broadly defined.

We could say "usually a string or integer identifier for the entity", even though thats probably implicit.

xtfer’s picture

Assigned: xtfer » Unassigned
jhodgdon’s picture

I'm sorry if my rather terse review came across as trolling. That was certainly not my intention. My intention was to make the documentation better.

The misuse of "id" in place of "ID" is unfortunately all over the Drupal Core documentation, and I apologize for stating that so tersely -- I get tired of saying this in about 30% of the patch reviews I do, and I probably said it less politely than I could have. It still needs to be fixed, however.

Regarding this patch... I see what you mean about "mixed" being the only option. However, I think we can also explain what this ID corresponds to on the entity in some way, like maybe something along the lines of "The unique identifier of the entity to load.", or whatever the ID would be called in the documentation for the Entity interface or base class for the ID methods or member variables? Or maybe just say "The ID" instead of "The id". Either way...

For extra credit, you could also fix the line above, which has another of my most-often-noted documentation review problems in it: mis-punctuation of "e.g." (which needs to be preceded by ; and followed by ,).

jhodgdon’s picture

Status: Needs work » Reviewed & tested by the community

Normally on documentation-only issues, people are willing to do a bit of extra clean-up and make the whole function have good documentation when a patch affects part of a function's documentation, and I am happy to commit patches that improve documentation.

But if you don't want to do that, I guess that is OK. Technically it is not part of this specific issue. I am putting it back to RTBC in case someone else wants to commit it.

Alternatively, if someone still wants to make a better patch that improves the docs of this function, that would also be most welcome, and I would be happy to review/commit such a patch.

jhodgdon’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x.

Status: Fixed » Closed (fixed)

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