API page: https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21TypedData...

TranslatableInterface::addTranslation() returns what ContentEntityStorageInterface::createTranslation() returns, and the documentation there says:

> A new entity translation object.

That's more accurate than 'this', since it's not the original object, but a clone (https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...)

Issue fork drupal-3226716

Command icon Show commands

Start 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:

About issue forks
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

guilhermevp made their first commit to this issue’s fork.

beatrizrodrigues made their first commit to this issue’s fork.

beatrizrodrigues opened merge request !1720

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
Status: Active » Needs review

Hope it fits.

lucienchalom’s picture

lucienchalom CreditAttribution: lucienchalom at CI&T commented
Status: Needs review » Reviewed & tested by the community 
   * @return \Drupal\Core\Entity\ContentEntityInterface
   *   A new entity translation object.

The documentation follows the suggested solution.
Moving to RTBC.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: return doc for TranslatableInterface::addTranslation() should not say 'this' » Missing return value documentation for TranslatableInterface::addTranslation()
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Needs work

Thanks for reporting and fixing this!

Removing credit for guilhermevp who did not actually make any changes in the MR.

It looks like there's a second instance of the exact same problem in a couple other places in the same interface. Can we fix those too?

beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
Assigned: Unassigned » beatrizrodrigues
beatrizrodrigues’s picture

beatrizrodrigues
She/Her
CreditAttribution: beatrizrodrigues at CI&T commented
Assigned: beatrizrodrigues » Unassigned
Status: Needs work » Needs review
joachim’s picture

joachim CreditAttribution: joachim at Factorial GmbH commented
Status: Needs review » Reviewed & tested by the community

LGTM.

quietone’s picture

quietone CreditAttribution: quietone commented

This does fix all the @return $this in \Drupal\Core\TypedData\TranslatableInterface to return the correct interface as well as adding the comment line. All feedback addressed.

This looks ready to commit to me. I'll wait for another committer to confirm.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Related issues: +#2932049: Change TranslatableStorageInterface::createTranslation() to accept TranslatableInterface

Thanks, good to fix them all at once.

On re-reviewing this I wondered if ContentEntityInterface was too specific, and if it should instead be TranslatableRevisionableInterface. I don't know the API well enough to say for sure.

Also found #2932049: Change TranslatableStorageInterface::createTranslation() to accept TranslatableInterface which is about a similar issue where the type might be overly specific.

  • quietone committed cffbfb6 on 10.0.x 
    Issue #3226716 by beatrizrodrigues, joachim, xjm, lucienchalom: Missing...

  • quietone committed 4657466 on 9.4.x 
    Issue #3226716 by beatrizrodrigues, joachim, xjm, lucienchalom: Missing...

  • quietone committed aeed17d on 9.3.x 
    Issue #3226716 by beatrizrodrigues, joachim, xjm, lucienchalom: Missing...
quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 10.0.x and cherry-picked to 9.4.x and 9.3.x.

Thank you!

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Related issues: +#3269177: Should some TranslatableInterface methods return ContentEntityInterface or TranslatableRevisionableInterface

I discussed #14 with xjm and although there is a question about the which interface to use this change is an improvement. I made a followup to answer the query, #3269177: Should some TranslatableInterface methods return ContentEntityInterface or TranslatableRevisionableInterface.

Status: Fixed » Closed (fixed)

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