Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#3 | drupal8.entity-system.2073901-3.patch | 788 bytes | xtfer |
#1 | drupal8.entity-system.2073901-1.patch | 11.61 KB | xtfer |
Comments
Comment #1
xtfer CreditAttribution: xtfer commentedPatch!
Comment #2
xtfer CreditAttribution: xtfer commentedWrong patch.
Comment #3
xtfer CreditAttribution: xtfer commentedComment #4
xtfer CreditAttribution: xtfer commentedComment #5
larowlanunless bot disagrees, config entities have (string) machine names for ids, so this is confusing
Comment #6
tim.plunkettWhy not
string|int
then? When I see mixed I generally think stdClass...Comment #7
larowlanyeah fair point
Comment #8
xtfer CreditAttribution: xtfer commentedIf 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.
Comment #9
larowlanPatch at #3 is consistent with https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
Comment #10
larowlanThats a link to EntityStorageControllerInterface::load
Comment #11
jhodgdonUm. 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?
Comment #12
xtfer CreditAttribution: xtfer commentedThats an issue with the EntityStorageControllerInterface, since it will (in fact) take any argument.
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.
Comment #13
xtfer CreditAttribution: xtfer commentedComment #14
jhodgdonI'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 ,).
Comment #15
jhodgdonNormally 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.
Comment #16
jhodgdonCommitted to 8.x.