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.
Files that need converting are:
- core/modules/field_ui/field_ui.admin.in
- core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.php
- core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.php
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#40 | 1999346-field-ui-request-40.patch | 4.08 KB | kim.pepper |
#32 | 1999346-field-ui-request-32.patch | 4.07 KB | effulgentsia |
#32 | interdiff.txt | 6.02 KB | effulgentsia |
#24 | 1999346-field-ui-request-24.patch | 7.69 KB | kim.pepper |
#24 | interdiff.txt | 5.04 KB | kim.pepper |
Comments
Comment #1
atchijov CreditAttribution: atchijov commentedComment #2
atchijov CreditAttribution: atchijov commentedatchijov & johnbickar working on this.
Comment #3
John Bickar CreditAttribution: John Bickar commentedPer atchijov, changes to core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.php depend on drupal_get_destination() from core/includes/common.inc
https://drupal.org/node/1998696
Comment #4
atchijov CreditAttribution: atchijov commentedComment #6
atchijov CreditAttribution: atchijov commented#4: drupal-Use_Symfony_Request_for_fieldui_module-1999346-3.patch queued for re-testing.
Comment #8
atchijov CreditAttribution: atchijov commentedsorry for typo 'destination' vs 'destinations'
Comment #9
kim.pepperLooking good. Just a few tweaks.
Let's move $request into it's own variable.
Same again
Why are we removing this here?
Comment #10
kim.pepperatchijov are you still working on this?
Comment #11
kim.pepperRe-rolled. There are a number of changes since the last patch. This one starts from the begging to remove the super globals. No interdiff unfortunately since the last patch didn't apply anymore.
Comment #13
piyuesh23 CreditAttribution: piyuesh23 at QED42 commentedRerolled the patch. Attaching the fixed patch.
Comment #14
piyuesh23 CreditAttribution: piyuesh23 commentedComment #16
kim.pepperRe-roll.
Comment #17
piyuesh23 CreditAttribution: piyuesh23 commentedFixing the patch and re-queuing for testing.
Comment #18
piyuesh23 CreditAttribution: piyuesh23 commentedPlease ignore the previous patch. #16 works fine. Din't refresh the page before posting my previous patch.
Comment #19
dawehnerGreat
Comment #21
dawehnerSome improvements.
Comment #23
kim.pepperUse $request->query for destination param.
Comment #24
kim.pepperDiscussed with Damz in IRC and agreed to pass in Request as a param to FieldUI::getDestination().
Comment #25
Damien Tournoud CreditAttribution: Damien Tournoud commentedThis violates
FormInterface
, by adding an additional required parameter ($request
) and hides the problem under the carpet by marking it as optional while it is not. I know we have other cases of this anti-pattern, but it is not a reason not to do it correctly.Both
FieldEditForm
andFieldInstanceEditForm
implementControllerInterface
, so let's just inject the request directly in the constructor.Comment #26
andypostUse $this->request in function body
Comment #27
kim.pepperI would prefer to stick to passing in the $request to buildForm() for consistency across the rest of core. Passing in request params, entities etc, is currently done by using buildForm() optional params.
Damz, can you post a separate issue for your suggested change?
Comment #28
dawehner@Damz
I can understand your problem here but if we drop using it at all then this should be discussed somewhere else, as this would also mean that we drop the usage of the controller resolver for buildForm().
Comment #29
Damien Tournoud CreditAttribution: Damien Tournoud commentedIn that particular case, both those forms already implement
ControllerInterface
, so I don't see the point of not injecting the request through the other mechanism (not that I approve of hardwiring service names in a static method).Comment #30
andypostAnother approach suggested in #2059245-18: Add a FormBase class containing useful methods
Comment #31
effulgentsia CreditAttribution: effulgentsia commentedComment #32
effulgentsia CreditAttribution: effulgentsia commentedUpdated for #2059245: Add a FormBase class containing useful methods.
Comment #33
yched CreditAttribution: yched commentedLooks good to me if green ?
Comment #35
jibranRelated #2071493: Modernize field_ui.module forms.
Comment #36
effulgentsia CreditAttribution: effulgentsia commentedYeah, fixing the test failures here requires changes that overlap with that one. Since that one is RTBC, postponing this one.
Comment #37
effulgentsia CreditAttribution: effulgentsia commentedComment #38
effulgentsia CreditAttribution: effulgentsia commented#32: 1999346-field-ui-request-32.patch queued for re-testing.
Comment #40
kim.pepperRe-roll #32
Comment #41
dawehnerPerfect!
Comment #42
webchickLooks like Damien's previous feedback was addressed in the latest version of this patch.
Committed and pushed to 8.x. Thanks!
Comment #43.0
(not verified) CreditAttribution: commentedrelated