Follow up for #1810370: Entity Translation API improvements
Updated: Comment #0
Problem/Motivation
some docs missing or mismatched, like @param, @return
mis-spelling and whitespace
Proposed resolution
added and matched
Remaining tasks
TBD
User interface changes
No.
API changes
No.
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#25 | 2031917-entity-translation-docs-improvements-25.patch | 2.09 KB | penyaskito |
| |||
#7 | interdiff-2031917-6-7.txt | 581 bytes | fgm |
#7 | 0001-Patch-2031917-7.patch | 4.69 KB | fgm |
#6 | interdiff-2031917-4-6.txt | 1.42 KB | fgm |
#6 | 0001-Patch-2031917-6.patch | 4.06 KB | fgm |
Comments
Comment #1
YesCT CreditAttribution: YesCT commented@param does not match.
the parameter list changed here. but there are no @param's. I checked:
class Entity implements IteratorAggregate, EntityInterface {
and
interface EntityInterface extends ComplexDataInterface, AccessibleInterface, TranslatableInterface {
and
the docs on the TranslatableInterface getTranslation() will work. the return is not as specific, but it's still accurate and I think that is what we are doing. (I asked @fubhy)
changing this to inheritdoc.
missing @return. added it.
corrects the @return, but is missing the @param Added it.
Comment #2
plachThanks :)
Please revert this, it's meant to cast the return type from
TranslatableInterface
toEntityInterface
.In other parts of the docs we refer to it as to the "The translation object".
Comment #3
YesCT CreditAttribution: YesCT commentedOK. I was wondering what to do when the docs should be the same, but the type of the return thing is known to be more specific.
Will add the @param then too.
Comment #4
YesCT CreditAttribution: YesCT commentedAh. I had noted 1. in #1 but not done it. It's done now.
Fixed the things from #2.
--
I didn't change missing docs in the BC decorator, since that will be pulled out.
--
One more grammar fix for "Checks translation statuses and invoke the ..." to invokes.
Comment #5
plachSorry, I had missed some stuff:
I am not sure we should write inline comments for the PHP docs themselves :)
This should be something like: "The translation object corresponding to the specified language. A new translation is created if missing."
The translation object.
translation object
Comment #6
fgmRerolled on today's HEAD.
Also, the examples in
hook_ENTITY_TYPE_translation_delete()
andhook_entity_translation_delete()
had become incorrect in the meantime, so updated them to work on current HEAD.Interdiff is to the rerolled version of the patch in #4.
Comment #7
fgmForgot to apply Plach's second suggestion. The three others no longer seem to be relevant due to the changes in core wording.
Comment #8
plachThis was supposed to document the
::getTranslation()
method, but it no longer applies, we can safely ignore it.Comment #9
fgmActually, language() will really create a new Language (not translation) object if it does not exist, if we look at
Entity::language()
. So maybe like this ?Comment #10
BerdirThat seems irrelevant and is an implementation detail. All you need to know is that you get a language object back.
The create new code there is for weird scenarios when a language doesn't exist anymore or so, to ensure that you always get an object back.
Comment #11
plachIn the translation case it was important to mention that a new entity translation is created if the langcode specified has not a corresponding translation already. In this case it's just an implementation detail, so I don't think it needs to be documented.
Comment #12
fgmThen #6 is RTBC since it includes everything but that unneeded change ?
Comment #25
penyaskitoDoc pieces from #6 that are still relevant.
Comment #26
smustgrave CreditAttribution: smustgrave at Mobomo commentedReading the comments and seems agreement in #6 is the way to go. #25 being a reroll of it.
Comment #27
quietone CreditAttribution: quietone at PreviousNext commentedI skimmed the issue and then read the patch. I checked that all comments have been addressed and they have been. The changes here are for grammar and coding standards. There are no changes for any of the problems or proposed resolution listed in the Issue Summary, although it was probably true when the issue was created. Other than that I think this can be committed.
I'll wait a day or two for any feedback.
Comment #29
quietone CreditAttribution: quietone at PreviousNext commentedCommitted 72f1f1f and pushed to 10.1.x. Thanks!
Comment #30
penyaskito