Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Comment | File | Size | Author |
---|---|---|---|
#33 | system-1925660-33.patch | 68.04 KB | tim.plunkett |
#33 | interdiff.txt | 13.91 KB | tim.plunkett |
#31 | system-1925660-31.patch | 67.5 KB | tim.plunkett |
#31 | interdiff.txt | 16.56 KB | tim.plunkett |
#28 | interdiff.txt | 2.79 KB | heddn |
Comments
Comment #1
tim.plunkettIncludes #1921996: Convert system_config_form() to implement FormInterface as a base class. and #1925140: drupal_form_submit() does not accept FormInterface objects
Comment #2
tim.plunkettAttempted reroll after #1763640: Introduce config context to make original config and different overrides accessible.
Comment #3
tim.plunkettThis needs a reroll, use #1925738: Convert language's system_config_form() to SystemConfigFormBase as a guide
Comment #4
ACF CreditAttribution: ACF commentedUpdated.
Comment #6
ACF CreditAttribution: ACF commentedUpdated for 'route_name'.
Comment #8
ACF CreditAttribution: ACF commented#6: 1925660-forminterface-system-drupal-6.patch queued for re-testing.
Submitting for re-test as the repo failed to checkout from git.
Comment #10
ACF CreditAttribution: ACF commentedUpdated, think I found the error.
Comment #12
ACF CreditAttribution: ACF commented#10: 1925660-forminterface-system-drupal-10.patch queued for re-testing.
Have gone for a retest because I can't replicate this issue locally. Although the three fails did relate to the admin page tests, which would definitely make sense.
Comment #13
tim.plunkett#10: 1925660-forminterface-system-drupal-10.patch queued for re-testing.
Comment #15
mtiftWorking on a re-roll
Comment #16
mtiftMy first attempt at a re-roll using git merge
Comment #17
star-szrThanks @mtift! It looks like there is a hunk missing from the reroll, just a deletion (see attached). The difference in patch file size was my tip-off.
I did a git blame on system.admin.inc and saw that the conflict came from #1926228: Performance page provides incorrect/incomplete information about CSS and JS compression (first patch), so I think that committed string change should be made in this patch as well.
Comment #18
ACF CreditAttribution: ACF commentedre-rolled
Comment #20
ACF CreditAttribution: ACF commentedOops, think I spotted the mistake.
Comment #22
ACF CreditAttribution: ACF commentedShould pass now.
Comment #23
mtiftThis is probably too nit picky, especially since #1624564: Coding standards for "use" statements (and elsewhere) discussed ordering "use statements" without coming to a resolution. That said, it seems like we could at least keep the Drupal and Symfony statements together, which is the only change in the attached patch.
The rest looks good to me, but I don't feel qualified to RTBC these.
Comment #24
tim.plunkettClose, some of this is just out of date due to other improvements.
This should call parent::__construct()
This should use the chained approach, you don't need the $config variable.
This should have the cache() injected, or at least \Drupal::cache().
This can all go away, this was moved into SystemConfigFormBase directly
Here, and all of the rest, don't need access arguments anymore
Comment #25
heddnComment #26
heddnComment #28
heddnIt would appear that the date configuration isn't included in the conversion effort on this issue. Let's try testing things again.
Comment #30
tim.plunkettAdding system_image_toolkit_settings since it was added in #1664844: Convert image toolkits into plugins
Comment #31
tim.plunkettThe last config form in system.module is taken care of by #1992606: Convert system_theme_settings to FormInterface
Comment #32
ParisLiakos CreditAttribution: ParisLiakos commentedso is it state not store:)
oh, does this finally work? am i outdated?:P
i wonder if we could use #attached here on the $form instead
cant we inject this?
inject path alias manager?
i cant see why those are removed
Comment #33
tim.plunkettThanks @ParisLiakos!
Comment #34
ParisLiakos CreditAttribution: ParisLiakos commentedlooks awesome now
also manually tested, and everything looks good
thanks!
Comment #35
catchCommitted/pushed to 8.x, thanks!