When I ran into #2584837: Double translation in template_preprocess_field_multiple_value_form() I realized \Drupal\Core\Field\BaseFieldDefinition::getLabel() returns TranslatableMarkup object while \Drupal\Core\Field\FieldConfigBase::getLabel() returns a string.

Docs in \Drupal\Core\TypedData\DataDefinitionInterface::getLabel() mention string only. PHPDoc comment should be updated to include possibility of an instance of TranslatableMarkup being returned too.

CommentFileSizeAuthor
#6 interdiff.txt780 bytesslashrsm
#6 2585193_6.patch683 bytesslashrsm
#3 2585193_3.patch660 bytesslashrsm
#2 2585193_2.patch1.53 KBslashrsm
#2 2585193_2_TEST_ONLY.patch993 bytesslashrsm
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

slashrsm created an issue. See original summary.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
Status: Active » Needs review
FileSize
2585193_2_TEST_ONLY.patch993 bytes
2585193_2.patch1.53 KB

Attached patch goes with the approach nr. 1. I am not sure it is the best solution, but it was easiest to show what is wrong (as it includes a test too).

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
Title: \Drupal\Core\Field\BaseFieldDefinition::getLabel() returns TranslatableMarkup instead of a string » \Drupal\Core\TypedData\DataDefinitionInterface::getLabel() needs to be updated
Component: field system » documentation
Category: Bug report » Task
Issue summary: View changes
Issue tags: +Documentation
FileSize
2585193_3.patch660 bytes

Updated summary and new patch based on discussion with @berdir on IRC.

The last submitted patch, 2: 2585193_2_TEST_ONLY.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Status: Needs review » Needs work

Good idea to document this, thanks!

But this now reads:

  /**
   * Returns a human readable label.
   *
   * String or an instance of TranslatableMarkup based on the way how the label
   * translation is handled.
   *
   * @return string|\Drupal\Core\StringTranslation\TranslatableMarkup
   *   The label.
   */

Two problems: (a) The added paragraph of docs is not a sentence (has no verb). (b) Normally we would want documentation that only pertains to the return value to be part of the @return docs not a stand-alone paragraph.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm as a volunteer commented
Status: Needs work » Needs review
FileSize
2585193_6.patch683 bytes
interdiff.txt780 bytes

Thank you for your suggestion. Fixed!

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community

Thank you this looks good.

The last submitted patch, 2: 2585193_2_TEST_ONLY.patch, failed testing.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented

Thanks! This is OK. I would have started the second sentence there with "A" but it is more or less OK as it is.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +rc eligible

I added @jhodgdon's A on commit. Committed 5911714 and pushed to 8.0.x. Thanks!

  • alexpott committed 5911714 on 8.0.x 
    Issue #2585193 by slashrsm, jhodgdon: \Drupal\Core\TypedData\...

Status: Fixed » Closed (fixed)

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