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
#8 3113074-sub-classing-file-8.patch5.06 KBgausarts
#2 blazy-Fix_subclassing_and_stop_overriding_constructors_in_blazy_Plugin_Field_FieldFormatter-3113074-2-D8.patch2.57 KBthalles

Comments

thalles created an issue. See original summary.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Status: Needs work » Needs review
StatusFileSize
new blazy-Fix_subclassing_and_stop_overriding_constructors_in_blazy_Plugin_Field_FieldFormatter-3113074-2-D8.patch2.57 KB

Follow a patch!

gausarts’s picture

gausarts commented
Issue summary: View changes

Thank you! Added https://www.drupal.org/core/d8-bc-policy to summary to back up the motivation.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Title: Fix subclassing and stop overriding constructors in blazy\Plugin\Field\FieldFormatter » Fix subclassing and stop overriding constructors in blazy\Plugin\Field\FieldFormatter\BlazyFileFormatterBase
gausarts’s picture

gausarts commented
Status: Needs review » Postponed (maintainer needs more info)

@thalles, perhaps we should consider setters specific for $instance->formatter and $instance->blazyManager, to allow sub-classes to override them and not even use ::create() unless necessary, just set it. If you could verify against Slick field formatters with their setters in the least, that will be very much appreciated. What do you think?

gausarts’s picture

gausarts commented

We should also consider starting from Slick to avoid breakage, if any (I haven't applied your patches, so no idea about potential issues, yet), with this change.
Then once Slick is done with its conversion, we can continue at Blazy.

thalles’s picture

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

Ok!

gausarts’s picture

gausarts commented
Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new 3113074-sub-classing-file-8.patch5.06 KB

We'll include the deprecated file formatter with this.

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

gausarts commented
Status: Needs review » Fixed

We abandoned setters idea during this transition. Maybe next time when we are settled with sub-modules.
Also included OEmbed related file formatter.

We got a release to catch up. Let's get the ball rolling. Improvements are very much welcome and may follow as always.

Committed for wider feedback. Feel free to re-open if any (side) issue. Thank you for contribution!

Status: Fixed » Closed (fixed)

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