Support from Acquia helps fund testing for Drupal Acquia logo

Comments

atchijov’s picture

Assigned: Unassigned » atchijov
atchijov’s picture

atchijov & johnbickar working on this.

John Bickar’s picture

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

atchijov’s picture

Status: Active » Needs review
FileSize
2.72 KB

Status: Needs review » Needs work

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

atchijov’s picture

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.

atchijov’s picture

Status: Needs work » Needs review
FileSize
3.2 KB

sorry for typo 'destination' vs 'destinations'

kim.pepper’s picture

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?

kim.pepper’s picture

atchijov are you still working on this?

kim.pepper’s picture

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

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.

piyuesh23’s picture

Rerolled the patch. Attaching the fixed patch.

piyuesh23’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
2.93 KB

Re-roll.

piyuesh23’s picture

Status: Needs review » Needs work
FileSize
3.67 KB

Fixing the patch and re-queuing for testing.

piyuesh23’s picture

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

dawehner’s picture

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.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
4.16 KB

Some improvements.

Status: Needs review » Needs work

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
798 bytes
4.24 KB

Use $request->query for destination param.

kim.pepper’s picture

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

Damien Tournoud’s picture

Status: Needs review » Needs work
   /**
    * {@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.

andypost’s picture

@@ -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

kim.pepper’s picture

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?

dawehner’s picture

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

Damien Tournoud’s picture

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

andypost’s picture

effulgentsia’s picture

Component: base system » field_ui.module
effulgentsia’s picture

Status: Needs work » Needs review
FileSize
6.02 KB
4.07 KB
yched’s picture

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.

jibran’s picture

effulgentsia’s picture

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.

effulgentsia’s picture

Status: Postponed » Needs work
effulgentsia’s picture

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.

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
4.08 KB

Re-roll #32

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Perfect!

webchick’s picture

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.

Anonymous’s picture

Issue summary: View changes

related