Problem/Motivation

UpdateSettingsForm inherited from ConfigFormBase which needs ConfigFactoryInterface as argument but it is not set, the form works because config has fallback to global \Drupal

Proposed resolution

Call parent static create($container) in own overridden method to comform interface and inheritance

Remaining tasks

decide on approach (comment #9) and commit

User interface changes

no

API changes

construction of form may change but constructors are not a part of BC

Data model changes

no

Release notes snippet

CommentFileSizeAuthor
#11 interdiff-3101138-4-3101138-10.txt543 bytespratik_kamble
#11 3101138-10.patch1017 bytespratik_kamble
#6 3101138-5.patch1.37 KBkishor_kolekar
#6 interdiff.txt1.24 KBkishor_kolekar
#4 3101138-4.patch1.02 KBandypost
#4 interdiff.txt385 bytesandypost
#3 3101138-3.patch1019 bytesandypost
#2 3101138-2.patch609 bytesandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs review
FileSize
3101138-2.patch609 bytes

For 8.x it could use this patch to keep BC

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
3101138-3.patch1019 bytes

Or could be like that patch

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt385 bytes
3101138-4.patch1.02 KB

Missed return

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Needs work
Issue tags: +Needs tests

Looks the form missing tests (only in core/modules/update/tests/src/Functional/UpdateCoreTest.php it used to get)

kishor_kolekar’s picture

kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 commented
Assigned: Unassigned » kishor_kolekar
Status: Needs work » Needs review
FileSize
interdiff.txt1.24 KB
3101138-5.patch1.37 KB
kishor_kolekar’s picture

kishor_kolekar CreditAttribution: kishor_kolekar as a volunteer and at QED42 commented
Assigned: kishor_kolekar » Unassigned
pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented

@kishor_kolekar, Patch by @andypost is the correct way to extend classes. FYR: https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs review » Needs work
Issue tags: -Needs tests 
+++ b/core/modules/update/src/UpdateSettingsForm.php
@@ -23,23 +23,15 @@ class UpdateSettingsForm extends ConfigFormBase implements ContainerInjectionInt
   public static function create(ContainerInterface $container) {
-    return new static(
-      $container->get('email.validator')
+    $instance = new static(
+      $container->get('config.factory')
     );
+    $instance->emailValidator = $container->get('email.validator');
+    return $instance;
   }

If we're going to do this to insulate ourselves from upstream constructor changes then we need to do $instance = parent::create(); - otherwise we might as well do the patch in #6

@andypost I think that as long as we hit the route in a test then we're okay because that means the form is being constructed and so we have test coverage of the changes here.

pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
Assigned: Unassigned » pratik_kamble
pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
FileSize
3101138-10.patch1017 bytes
interdiff-3101138-4-3101138-10.txt543 bytes

Rerolled patch.

pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
Status: Needs work » Needs review
pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
Issue tags: +GlobalContributionWeekend2020, +GCWIndia2020, +punedrupalgroup
C-Logemann’s picture

C-Logemann
Primary language German
Location Frankfurt/M, Germany
CreditAttribution: C-Logemann at Nodegard commented
Issue tags: -GlobalContributionWeekend2020 +ContributionWeekend2020

Fixing issue tag to "ContributionWeekend2020“ as suggested on global event page.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

andypost’s picture

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

fixed summary

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed f737942 and pushed to 9.0.x, backported to 8.9.x

  • catch committed f737942 on 9.0.x 
    Issue #3101138 by andypost, pratik_kamble, kishor_kolekar, alexpott:...

  • catch committed a658288 on 8.9.x 
    Issue #3101138 by andypost, pratik_kamble, kishor_kolekar, alexpott:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Title: UpdateSettingsForm should call parent constructor » UpdateSettingsForm should not override parent constructor

Retrospective title change...

Status: Fixed » Closed (fixed)

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