Problem/Motivation

abstract class FormatterBase extends PluginSettingsBase implements FormatterInterface, ContainerFactoryPluginInterface {

class LinkFormatter extends FormatterBase implements ContainerFactoryPluginInterface {
class StringFormatter extends FormatterBase implements ContainerFactoryPluginInterface {
...

Proposed resolution

Remove redundant ContainerFactoryPluginInterface

CommentFileSizeAuthor
#12 raw-interdiff-2-11.txt265 bytesjungle
#12 3112328-9.0.x-11.patch10.04 KBjungle
#2 redundant-interface-3112328-2.patch10.06 KBjungle
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jungle created an issue. See original summary.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
FileSize
redundant-interface-3112328-2.patch10.06 KB

Checked and removed from files below. It might happen in other similar subclasses of non-FormatterBase. But as the title indicated, let this issue focus on subclasses of FormatterBase unless changing the title.

  • core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/EntityReferenceEntityFormatter.php
  • core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/StringFormatter.php
  • core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
  • core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampFormatter.php
  • core/modules/comment/src/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
  • core/modules/content_moderation/src/Plugin/Field/FieldFormatter/ContentModerationStateFormatter.php
  • core/modules/datetime/src/Plugin/Field/FieldFormatter/DateTimeFormatterBase.php
  • core/modules/image/src/Plugin/Field/FieldFormatter/ImageFormatter.php
  • core/modules/link/src/Plugin/Field/FieldFormatter/LinkFormatter.php
  • core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php
  • core/modules/responsive_image/src/Plugin/Field/FieldFormatter/ResponsiveImageFormatter.php
jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Active » Needs review
jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Assigned: jungle » Unassigned
hash6’s picture

hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commented
Assigned: Unassigned » hash6
hash6’s picture

hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commented
Assigned: hash6 » Unassigned
Status: Needs review » Reviewed & tested by the community

Since FormatterBase already implements ContainerFactoryPluginInterface , we don't need to implement it in the following classes which extends FormatterBase(or extends class which ultimately extends FormatterBase)

  • EntityReferenceEntityFormatter
  • StringFormatter
  • TimestampAgoFormatter
  • TimestampFormatter
  • CommentDefaultFormatter
  • ContentModerationStateFormatter
  • DateTimeFormatterBase
  • ImageFormatter
  • LinkFormatter
  • OEmbedFormatter
  • ResponsiveImageFormatter
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

RTBC +1 (all formatters are fixed)

PS: would be great to automate detection of that for other plugins

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.8.x-dev » 8.9.x-dev

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: ContainerFactoryPluginInterface is already announced in FormatterBase » FormatterBase implements ContainerFactoryPluginInterface, so it's not necessary for its child classes to repeat that they do as well
xjm’s picture

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

Nice catch!

I checked the inheritance on all those and confirmed that they're descended from FormatterBase one way or another.

I can't think of anything this would disrupt. I did some archaeology as to why we have this state to begin with and it looks like #3051633: Add ContainerFactoryPluginInterface to Drupal\Core\Field\WidgetBase and Drupal\Core\Field\FormatterBase was only committed last summer, but the various implementations added the interface much earlier, for example:

So this seems fine as a cleanup.

That said, it does not apply to 9.1.x, so we need a D9 version.

Thanks!

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Needs work » Needs review
FileSize
3112328-9.0.x-11.patch10.04 KB
raw-interdiff-2-11.txt265 bytes

Thanks @hash6, @andypost and @xjm for reviewing!

A patch rerolled from #2 against 9.0.x

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Related issues: +#3133122: [policy][no patch] Adjust policy of handling coder relevant Drupal Core issues

Ideally, this could be done by a phpcs sniffer, but it would take longer to land, So I just filed an issue for further discussion of the policy of handling such kind of issues. #3133122: [policy][no patch] Adjust policy of handling coder relevant Drupal Core issues

Comments are welcome! Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

Back to RTBC

  • xjm committed b286133 on 9.1.x 
    Issue #3112328 by jungle, hash6, xjm: FormatterBase implements...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +Needs change record

Alright, pushed to 9.1.x. This change is theoretically BC, but we might want to make it 9.1.x-only and create a CR explaining why this change was made, referencing #3051633: Add ContainerFactoryPluginInterface to Drupal\Core\Field\WidgetBase and Drupal\Core\Field\FormatterBase

Leaving at 9.1.x now for commit. Thanks!

Status: Fixed » Closed (fixed)

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