Deprecate InlineFormInterface::getEntityTypeLabels()

Created on 10 January 2024, 6 months ago
Updated 1 February 2024, 5 months ago

There's an old @todo in InlineFormInterface::getEntityTypeLabels():

Remove when #1850080 lands and IEF starts requiring Drupal 8.1.x

#1850080: Entity type labels lack plurality, cannot generate UI text based on label if plural is needed β†’ is the issue that added the entity singular, plural, and count label formatters. It was committed years ago. It looks like this function was added to work around the problem of not having those formatters back in the day.

Proposed Resolution

  1. No, we fixed that issue directly and let the folks be credited there.
  2. Deprecate InlineFormInterface::getEntityTypeLabels().
  3. (Done via πŸ“Œ Remove hardcoded word 'entities' in EntityInlineForm::getEntityTypeLabels() Fixed )
  4. Delete NodeInlineForm::getEntityTypeLabels().
  5. Draft a change record and add its URL to the @deprecated message before committing.
  6. Commit and publish the change record.
  7. Create a follow-up issue to remove the function in 4.x and have InlineEntityFormBase::getEntityTypeLabels() return the singular/plural entity type labels directly.
πŸ“Œ Task
Status

Needs review

Version

3.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States dcam

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

Merge Requests

Comments & Activities

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

    Updated the proposed resolution because I don't think we can change InlineEntityFormBase::getEntityTypeLabels() to use the entity label formatters yet. If we did, then no InlineFormInterface::getEntityTypeLabels() functions would be getting executed at all, breaking backward compatibility.

  • πŸ‡ΊπŸ‡ΈUnited States dcam

    Noted that a change record should be created.

  • Merge request !104Added deprecation and test β†’ (Open) created by dcam
  • Pipeline finished with Failed
    6 months ago
    Total: 388s
    #74663
  • Pipeline finished with Failed
    6 months ago
    Total: 683s
    #74668
  • Pipeline finished with Failed
    6 months ago
    Total: 476s
    #74686
  • Pipeline finished with Failed
    6 months ago
    Total: 471s
    #74696
  • Pipeline finished with Failed
    6 months ago
    Total: 506s
    #75137
  • Pipeline finished with Failed
    6 months ago
    Total: 490s
    #75144
  • Pipeline finished with Success
    6 months ago
    Total: 528s
    #75149
  • Status changed to Needs review 6 months ago
  • πŸ‡ΊπŸ‡ΈUnited States dcam

    The MR covers items 2, 3, and 4 from the proposed resolution. It even has a deprecation test, per Drupal's deprecation policy. The majority of the changes resulted from the removal of NodeInlineForm::getEntityTypeLabels() because Core tried to get away from using the word "node" on the front end in favor of "content item." So all of the assertions that were looking for the word "node" in the UI had to be updated.

  • πŸ‡ΊπŸ‡ΈUnited States dcam

    I should have searched for existing issues. Shame on me. See #3051925: deal with TODO for 1850080 β†’ .

  • Status changed to Needs work 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States dww

    This needs a rebase / reroll now that πŸ“Œ Remove hardcoded word 'entities' in EntityInlineForm::getEntityTypeLabels() Fixed is committed.

    Thanks,
    -Derek

  • Pipeline finished with Success
    5 months ago
    Total: 693s
    #85742
  • Pipeline finished with Success
    5 months ago
    Total: 434s
    #85750
  • Status changed to Needs review 5 months ago
  • πŸ‡ΊπŸ‡ΈUnited States dcam

    This was my first experience using the GitLab conflict resolver tool. At least in this case it made the resolution quick and easy, but that's at least partly because this MR contained the exact same changes that caused the conflict.

    I also took the time to resolve the PHPCS issues that were introduced by the patch. Although there are now two more issues, but that's due to me using placeholder text for the change record URL that needs to be added to the deprecated function. I haven't drafted a CR yet because there hasn't been any consensus about the change.

  • πŸ‡ΊπŸ‡ΈUnited States dww

    Great, thanks! A few updates to the summary. +1 to the plan. Feel free to draft a CR for this so we can get the link right and see a fully green pipeline here.

    Thanks again,
    -Derek

Production build 0.69.0 2024