Message:setLanguage() and Message::getLanguage() operate on the wrong property

Created on 10 June 2024, over 1 year ago

Problem/Motivation

The Message entity defines a langcode property, but the ::setLanguage() and ::getLanguage() methods operate on the language property which is not defined anywhere.

Would probably make sense to change these to update 'langcode' instead?

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

πŸ› Bug report
Status

Active

Version

1.0

Component

Code

Created by

πŸ‡¬πŸ‡§United Kingdom catch

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @catch
  • πŸ‡ΊπŸ‡ΈUnited States bluegeek9

    I think it is intentional. I think it is done this way so the message can be toggled te different languages.

    /**
       * Set the language that should be used.
       *
       * @param string $language
       *   The language to load from the message template when fetching the text.
       */
      public function setLanguage($language);
    
  • πŸ‡§πŸ‡¬Bulgaria pfrenssen Sofia

    I am also confused by this. On the surface it looks like this is rather useless. We allow setting a language in non-persisted state on the Message entity just to avoid having to pass it to Message::getTexts()? Adding an additional layer of complexity for this seems absurd. Our users could just pass the $langcode to ::getTexts() directly. Why would we add an ephemeral property on the entity to avoid passing it?

    There must be more to this story. I did some digging. The property and the getter have been introduced in this issue as part of the work to port the module from Drupal 7 to Drupal 8. At the time we had no experience using modern entities, and it looked like a great idea to introduce a __toString() method on the entity. It was probably envisioned that we could directly use the entity object in Twig templates, how cool would that be!

    Now with hindsight and having years of experience with building Drupal 8+ sites it is crystal clear rendering entities is dependent on a view mode, and needs to be done using the EntityViewBuilder. Rendering entities as strings was a fun idea but it doesn't make sense in modern Drupal. AFAIK no other entity types in core or popular contrib modules implement __toString().

    I propose to get rid of this. We should start by deprecating it, since there might be users in the wild who rely on this. Let's deprecate ::getLanguage(), ::setLanguage() and ::toString().

    We should also check which code relies on this and will need to be refactored. We should take a look at the GetText Views field handler and other modules like Message Notify.

  • πŸ‡§πŸ‡¬Bulgaria pfrenssen Sofia

    Yes it looks like Message Notify would need some work if we would go through with this. It is completely oblivious to language handling, it doesn't pass on a langcode when it renders the message in MessageNotifierBase::send().

Production build 0.71.5 2024