Part of #1998638: Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object

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

#2071493: Modernize field_ui.module forms

Files: 
CommentFileSizeAuthor
#40 1999346-field-ui-request-40.patch4.08 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 58,011 pass(es).
[ View ]
#32 1999346-field-ui-request-32.patch4.07 KBeffulgentsia
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1999346-field-ui-request-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#32 interdiff.txt6.02 KBeffulgentsia
#24 1999346-field-ui-request-24.patch7.69 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 57,838 pass(es), 105 fail(s), and 4 exception(s).
[ View ]
#24 interdiff.txt5.04 KBkim.pepper
#23 1999346-field-ui-request-23.patch4.24 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 57,705 pass(es).
[ View ]
#23 interdiff.txt798 byteskim.pepper
#21 drupal-1999346-21.patch4.16 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 56,772 pass(es), 43 fail(s), and 4 exception(s).
[ View ]
#21 interdiff.txt1.51 KBdawehner
#17 1999346-field-ui-request-16.patch3.67 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] 56,791 pass(es), 107 fail(s), and 541 exception(s).
[ View ]
#16 1999346-field-ui-request-16.patch2.93 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 56,317 pass(es).
[ View ]
#13 1999346-field-ui-request-13.patch3.67 KBpiyuesh23
FAILED: [[SimpleTest]]: [MySQL] 56,249 pass(es), 107 fail(s), and 541 exception(s).
[ View ]
#11 1999346-field-ui-request-11.patch3.65 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 57,460 pass(es), 91 fail(s), and 246 exception(s).
[ View ]
#8 drupal-Use_Symfony_Request_for_fieldui_module-1999346-8.patch3.2 KBatchijov
PASSED: [[SimpleTest]]: [MySQL] 55,765 pass(es).
[ View ]
#4 drupal-Use_Symfony_Request_for_fieldui_module-1999346-3.patch2.72 KBatchijov
FAILED: [[SimpleTest]]: [MySQL] 55,669 pass(es), 39 fail(s), and 4 exception(s).
[ View ]

Comments

Assigned:Unassigned» atchijov

atchijov & johnbickar working on this.

Per 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

Status:Active» Needs review
StatusFileSize
new2.72 KB
FAILED: [[SimpleTest]]: [MySQL] 55,669 pass(es), 39 fail(s), and 4 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, drupal-Use_Symfony_Request_for_fieldui_module-1999346-3.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal-Use_Symfony_Request_for_fieldui_module-1999346-3.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,765 pass(es).
[ View ]

sorry for typo 'destination' vs 'destinations'

Status:Needs review» Needs work

Looking good. Just a few tweaks.

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -451,9 +451,13 @@ function field_ui_get_destinations($destinations) {
-  $destinations = !empty($_REQUEST['destinations']) ? $_REQUEST['destinations'] : array();
+  $request_destination = \Drupal::request()->get('destinations');
+  $destinations = !empty($request_destination) ? $request_destination : array();
   if (!empty($destinations)) {
-    unset($_REQUEST['destinations']);
+    // This is closest to unset() I could think of.
+    // unset($_REQUEST['destinations']);
+    \Drupal::request()->query->remove('destinations');
+    \Drupal::request()->request->remove('destinations');

Let's move $request into it's own variable.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceEditForm.phpundefined
@@ -248,9 +248,10 @@ public function submitForm(array &$form, array &$form_state) {
+    $query_destination = \Drupal::request()->query->get('destination');
+    if (isset($query_destination)) {
       $destination = drupal_get_destination();
-      unset($_GET['destination']);
+      \Drupal::request()->query->remove('destination');

Same again

+++ b/core/modules/system/system.moduleundefined
@@ -1243,7 +1243,6 @@ function system_library_info() {
-      array('system', 'drupal.ajax'),

Why are we removing this here?

atchijov are you still working on this?

Status:Needs work» Needs review
Issue tags:+WSCCI-conversion
StatusFileSize
new3.65 KB
FAILED: [[SimpleTest]]: [MySQL] 57,460 pass(es), 91 fail(s), and 246 exception(s).
[ View ]

Re-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.

Status:Needs review» Needs work

The last submitted patch, 1999346-field-ui-request-11.patch, failed testing.

StatusFileSize
new3.67 KB
FAILED: [[SimpleTest]]: [MySQL] 56,249 pass(es), 107 fail(s), and 541 exception(s).
[ View ]

Rerolled the patch. Attaching the fixed patch.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1999346-field-ui-request-13.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.93 KB
PASSED: [[SimpleTest]]: [MySQL] 56,317 pass(es).
[ View ]

Re-roll.

Status:Needs review» Needs work
StatusFileSize
new3.67 KB
FAILED: [[SimpleTest]]: [MySQL] 56,791 pass(es), 107 fail(s), and 541 exception(s).
[ View ]

Fixing the patch and re-queuing for testing.

Please ignore the previous patch. #16 works fine. Din't refresh the page before posting my previous patch.

Status:Needs work» Reviewed & tested by the community

Great

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1999346-field-ui-request-16.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
new4.16 KB
FAILED: [[SimpleTest]]: [MySQL] 56,772 pass(es), 43 fail(s), and 4 exception(s).
[ View ]

Some improvements.

Status:Needs review» Needs work

The last submitted patch, drupal-1999346-21.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new798 bytes
new4.24 KB
PASSED: [[SimpleTest]]: [MySQL] 57,705 pass(es).
[ View ]

Use $request->query for destination param.

StatusFileSize
new5.04 KB
new7.69 KB
FAILED: [[SimpleTest]]: [MySQL] 57,838 pass(es), 105 fail(s), and 4 exception(s).
[ View ]

Discussed with Damz in IRC and agreed to pass in Request as a param to FieldUI::getDestination().

Status:Needs review» Needs work

<?php
  
/**
    * {@inheritdoc}
    */
-  public function buildForm(array $form, array &$form_state, FieldInstanceInterface $field_instance = NULL) {
+  public function
buildForm(array $form, array &$form_state, FieldInstanceInterface $field_instance = NULL, Request $request = NULL) {
    
$this->instance = $form_state['instance'] = $field_instance;
+   
$this->request = $request;
?>

This 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 and FieldInstanceEditForm implement ControllerInterface, so let's just inject the request directly in the constructor.

@@ -64,8 +72,9 @@ public static function create(ContainerInterface $container) {
-  public function buildForm(array $form, array &$form_state, FieldInstanceInterface $field_instance = NULL) {
+  public function buildForm(array $form, array &$form_state, FieldInstanceInterface $field_instance = NULL, Request $request = NULL) {
@@ -89,8 +96,9 @@ public function getFormID() {
-  public function buildForm(array $form, array &$form_state, FieldInstanceInterface $field_instance = NULL) {
+  public function buildForm(array $form, array &$form_state, FieldInstanceInterface $field_instance = NULL, Request $request = NULL) {

Use $this->request in function body

Status:Needs work» Needs review

I 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?

Status:Needs review» Needs work

@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().

In 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).

Component:base system» field_ui.module

Status:Needs work» Needs review
StatusFileSize
new6.02 KB
new4.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1999346-field-ui-request-32.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Reviewed & tested by the community

Looks good to me if green ?

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1999346-field-ui-request-32.patch, failed testing.

Status:Needs work» Postponed

Yeah, fixing the test failures here requires changes that overlap with that one. Since that one is RTBC, postponing this one.

Status:Postponed» Needs work

Status:Needs work» Needs review
Issue tags:-WSCCI-conversion

#32: 1999346-field-ui-request-32.patch queued for re-testing.

Status:Needs review» Needs work
Issue tags:+WSCCI-conversion

The last submitted patch, 1999346-field-ui-request-32.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.08 KB
PASSED: [[SimpleTest]]: [MySQL] 58,011 pass(es).
[ View ]

Re-roll #32

Status:Needs review» Reviewed & tested by the community

Perfect!

Status:Reviewed & tested by the community» Fixed

Looks like Damien's previous feedback was addressed in the latest version of this patch.

Committed and pushed to 8.x. Thanks!

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

Issue summary:View changes

related