Problem/Motivation

Add object that uses the FactoryInterface should have dependencies inject via ::create and not ::__construct(). This allows parent classes to change their constructor without breaking any code.
See https://events.drupal.org/seattle2019/sessions/drupal-9-coming-getting-y...
See https://www.drupal.org/core/d8-bc-policy

Example: \Drupal\config_sync\Controller\ConfigSyncController::create
See https://www.youtube.com/watch?time_continue=1554&v=hN9KjaBvAUk
See https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...

Proposed resolution

Fix subclassing and stop overriding constructors.

CommentFileSizeAuthor
#13 3113159-subclassing-13.patch6.75 KBgausarts
#8 slick-Fix_subclassing_and_stop_overriding_constructors_in_slick_Plugin_Field_FieldFormatter_SlickFileFormatter-3113159-8-D8.patch2.55 KBthalles
#5 slick-Fix_subclassing_and_stop_overriding_constructors_in_slick_Plugin_Field_FieldFormatter_SlickFileFormatter-3113159-5-D8.patch2.14 KBthalles
#3 slick-Fix_subclassing_and_stop_overriding_constructors_in_slick_Plugin_Field_FieldFormatter_SlickFileFormatter-3113159-3-D8.patch1.17 KBthalles

Comments

thalles created an issue. See original summary.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Project: Blazy » Slick Carousel
thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Status: Needs work » Needs review
StatusFileSize
new slick-Fix_subclassing_and_stop_overriding_constructors_in_slick_Plugin_Field_FieldFormatter_SlickFileFormatter-3113159-3-D8.patch1.17 KB

Follow a patch

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Assigned: Unassigned » gausarts
thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
StatusFileSize
new slick-Fix_subclassing_and_stop_overriding_constructors_in_slick_Plugin_Field_FieldFormatter_SlickFileFormatter-3113159-5-D8.patch2.14 KB

Follow a new patch

gausarts’s picture

gausarts commented

Thanks! Please consider setters as mentioned at Blazy for all plugin stuffs.

Few more issues:

  1. There is a small typo $$container.
  2. Needs setters for image.factory, formatter and manager.

We can remove later those defined at Blazy later on like image.factory. Until then, it might be needed during transition, unless you are confident it works without.

FYI, unlike Blazy, formatter and manager are branching out at sub-classes for their own needs, so explicit calls are required.

gausarts’s picture

gausarts commented
Assigned: gausarts » thalles
Status: Needs review » Needs work
thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
StatusFileSize
new slick-Fix_subclassing_and_stop_overriding_constructors_in_slick_Plugin_Field_FieldFormatter_SlickFileFormatter-3113159-8-D8.patch2.55 KB

Follow a new patch with a setter to $blazyOembed, I think formatter and manager will is set in slick\Plugin\Field\FieldFormatter\SlickFileFormatterBase.

See:

  /**
   * Constructs a SlickFileFormatterBase instance.
   */
  public function __construct($plugin_id, $plugin_definition, FieldDefinitionInterface $field_definition, array $settings, $label, $view_mode, array $third_party_settings, ImageFactory $image_factory, SlickFormatterInterface $formatter, SlickManagerInterface $manager) {
    parent::__construct($plugin_id, $plugin_definition, $field_definition, $settings, $label, $view_mode, $third_party_settings, $image_factory, $formatter);
    $this->formatter = $formatter;
    $this->manager   = $manager;
  }
thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Status: Needs work » Needs review
gausarts’s picture

gausarts commented

You are right. You may want to start from the base class then :)

gausarts’s picture

gausarts commented

I'll get back to you tomorrow. It's late here. Feel free to continue patching the rest, and we'll review back. Thanks!

gausarts’s picture

gausarts commented
Status: Needs review » Needs work

Feel free to commit this after the change mentioned here and there. Thanks a lot!

gausarts’s picture

gausarts commented
Assigned: thalles » Unassigned
Status: Needs work » Needs review
StatusFileSize
new 3113159-subclassing-13.patch6.75 KB

Added ::injectServices() for easy copy/paste through Slick formatters.
Let's see if any issue.

  • gausarts committed 0fc52d7 on 8.x-2.x authored by thalles 
    Issue #3113159 by thalles, gausarts: Fix subclassing and stop overriding...
gausarts’s picture

gausarts commented
Status: Needs review » Fixed

Let's get the ball rolling, we have another release to catch up. Any (side) issue and or improvements may follow as always.

Committed. Thank you for contribution!

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented

Thanks @gausarts!

gausarts’s picture

gausarts commented

I do thank you! You saved me from headaches with this.

Status: Fixed » Closed (fixed)

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