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
#9 3124257-9.scheduler-admin-form-subclassing.patch3.37 KBjonathan1055
#8 interdiff_5-8.txt1.07 KBbaikho
#8 scheduler-fix_subclassing-3124257-8.patch1.72 KBbaikho
#5 scheduler-Fix_subclassing_and_stop_overriding_constructors_in_scheduler_Form_SchedulerAdminForm-3124257-5-D8.patch1.67 KBthalles
#2 scheduler-Fix_subclassing_and_stop_overriding_constructors_in_scheduler_Form_SchedulerAdminForm-3124257-2-D8.patch1.64 KBthalles

Comments

thalles created an issue. See original summary.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
StatusFileSize
new scheduler-Fix_subclassing_and_stop_overriding_constructors_in_scheduler_Form_SchedulerAdminForm-3124257-2-D8.patch1.64 KB

Follow a patch!

thalles’s picture

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

Status: Needs review » Needs work

The last submitted patch, 2: scheduler-Fix_subclassing_and_stop_overriding_constructors_in_scheduler_Form_SchedulerAdminForm-3124257-2-D8.patch, failed testing. View results

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Status: Needs work » Needs review
StatusFileSize
new scheduler-Fix_subclassing_and_stop_overriding_constructors_in_scheduler_Form_SchedulerAdminForm-3124257-5-D8.patch1.67 KB

Ops! I forget the return!

Follow a new patch

chr.fritsch’s picture

chr.fritsch
he/him
Primary language German
Location 🇩🇪🇪🇺🌍
commented

I would propose to do that with protected setDateFormatter(DateFormatterInterface $date_formatter). It has the advantage that you don't lose the type checking.

jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Needs work

I would propose to do that with protected setDateFormatter

Therefore changing to 'needs work'

baikho’s picture

baikho commented
Status: Needs work » Needs review
StatusFileSize
new scheduler-fix_subclassing-3124257-8.patch1.72 KB
new interdiff_5-8.txt1.07 KB
jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new 3124257-9.scheduler-admin-form-subclassing.patch3.37 KB

Thanks @baikho for the re-patch, that is helpful.
In #6 @chr.fritsch said "I would propose to do that with protected setDateFormatter" so I have made that change here. I also noticed a @todo on the form, to remove the placeholder.

  • jonathan1055 committed a6e39fb on 8.x-1.x authored by thalles 
    Issue #3124257 by thalles, baikho, chr.fritsch: Fix subclassing and stop...
jonathan1055’s picture

jonathan1055 commented
Status: Needs review » Fixed
Parent issue: » #3136831: [meta] Drupal9 Readiness and compatibility - Scheduler

Fixed. Thank you @thalles, @chr.fritsch and @baikho. This will be in 8.x-1.3 which I should release today or tomorrow.

Status: Fixed » Closed (fixed)

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