Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#42 field-ui-1987708-42.patch8.62 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 58,128 pass(es).
[ View ]
#42 interdiff.txt8.67 KBtim.plunkett
#40 1987708-route-field-ui-40.patch8.02 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 57,588 pass(es).
[ View ]
#40 interdiff.txt742 bytesswentel
#37 1987708-route-field-ui-37.patch8.01 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 57,742 pass(es), 4 fail(s), and 1 exception(s).
[ View ]
#35 1987708-route-field-ui-35.patch7.96 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 55,062 pass(es).
[ View ]
#30 1987708-route-field-ui-30.patch8.02 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 55,513 pass(es).
[ View ]
#30 interdiff-27-30.txt2.92 KBsmiletrl
#29 1987708-route-field-ui-19.patch8.01 KBsmiletrl
PASSED: [[SimpleTest]]: [MySQL] 55,800 pass(es).
[ View ]
#29 interdiff-27-19.txt2.9 KBsmiletrl
#27 1987708-route-field-ui-27.patch8.2 KBmparker17
PASSED: [[SimpleTest]]: [MySQL] 55,719 pass(es).
[ View ]
#27 interdiff.txt1.84 KBmparker17
#20 1987708-route-field-ui-20.patch7.13 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 55,684 pass(es).
[ View ]
#20 1987708-20-interdiff.txt119 bytesmtift
#18 1987708-route-field-ui-18.patch7.13 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 55,685 pass(es).
[ View ]
#18 1987708-diff-12-18.txt444 bytesvijaycs85
#12 1987708-12.patch7.13 KBmtift
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.php.
[ View ]
#12 interdiff.txt210 bytesmtift
#11 1987708-11.patch7.07 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 55,916 pass(es).
[ View ]
#11 interdiff.txt117 bytesmtift
#9 1987708-9.patch7.08 KBmtift
PASSED: [[SimpleTest]]: [MySQL] 55,682 pass(es).
[ View ]
#9 interdiff.txt2.13 KBmtift
#3 1987708-3.patch6.34 KBswentel
FAILED: [[SimpleTest]]: [MySQL] 55,712 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Comments

Title:Convert field_test_entity_add() to a new style controllerConvert field_ui_fields_list() to a new style controller

Assigned:Unassigned» swentel
Issue tags:+Field API

Status:Active» Needs review
StatusFileSize
new6.34 KB
FAILED: [[SimpleTest]]: [MySQL] 55,712 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

+++ b/core/modules/field_ui/field_ui.moduleundefined
@@ -53,8 +53,7 @@ function field_ui_menu() {
   $items['admin/reports/fields'] = array(
     'title' => 'Field list',
     'description' => 'Overview of fields on all entity types.',
-    'page callback' => 'field_ui_fields_list',
-    'access arguments' => array('administer content types'),
+    'route_name' => 'field_list',
     'type' => MENU_NORMAL_ITEM,
     'file' => 'field_ui.admin.inc',

Not sure what is the current status of hook_menu() deprecation: for something that is just a MENU_NORMAL_ITEM (no integration with local tasks, etc), do we still need to keep an entry in hook_menu() ?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.phpundefined
@@ -0,0 +1,78 @@
+  public static function create(ContainerInterface $container) {
+    return new static();

This should inject the services needed in the fieldsList() method:
the entity manager,
the FieldInfo service,
(looks like that"s all for the moment)

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.phpundefined
+}
\ No newline at end of file

:-)

Status:Needs work» Needs review

#3: 1987708-3.patch queued for re-testing.

Assigned:swentel» Unassigned

Unassigning for now

Status:Needs review» Needs work

The last submitted patch, 1987708-3.patch, failed testing.

Assigned:Unassigned» mtift

I'll take a crack at this one

Assigned:mtift» Unassigned
Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.13 KB
new7.08 KB
PASSED: [[SimpleTest]]: [MySQL] 55,682 pass(es).
[ View ]

The attached patch incorporates comments from #4

Status:Reviewed & tested by the community» Needs review

Whoops. Needs review.

StatusFileSize
new117 bytes
new7.07 KB
PASSED: [[SimpleTest]]: [MySQL] 55,916 pass(es).
[ View ]

Renamed the _construct object

StatusFileSize
new210 bytes
new7.13 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.php.
[ View ]

Added a missing param thanks to @swentel

Status:Needs review» Needs work
Issue tags:-Field API, -WSCCI-conversion

The last submitted patch, 1987708-12.patch, failed testing.

Status:Needs work» Needs review

#12: 1987708-12.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1987708-12.patch, failed testing.

Status:Needs work» Needs review

#12: 1987708-12.patch queued for re-testing.

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

The last submitted patch, 1987708-12.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new444 bytes
new7.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,685 pass(es).
[ View ]

syntax error fix...

To address concern for #4,

<?php
  $items
['admin/reports/fields/list'] = array(
   
'title' => 'Entities',
   
'type' => MENU_DEFAULT_LOCAL_TASK,
  );
?>

There's such a local task, so perhaps that's why
'type' => MENU_NORMAL_ITEM,
will be required.

For

+  public function fieldsList() {
+    $instances = $this->fieldInfo->getInstances();
+    $field_types = field_info_field_types();
+    $bundles = entity_get_bundles();
+    $entity_manager = $this->entityManager;

I guess
$fields = $this->fieldInfo->getFieldMap();

could be a better option.

One thing: getInstances will have some performance issue,see Clas Fieldinfo. Second thing: Results from getFieldMap() could make more sense for this scenario. For what we really want here is fields list, not instance list.

+          // Initialize the row if we encounter the field for the first time.
+          if (!isset($rows[$field_name])) {
+            $rows[$field_name]['class'] = $field['locked'] ? array('menu-disabled') : array('');
+            $rows[$field_name]['data'][0] = $field['locked'] ? t('@field_name (Locked)', array('@field_name' => $field_name)) : $field_name;
+            $module_name = $field_types[$field['type']]['module'];
+            $rows[$field_name]['data'][1] = $field_types[$field['type']]['label'] . ' ' . t('(module: !module)', array('!module' => $modules[$module_name]->info['name']));
+          }

This if condition means there are redundant loops, because current code loops all existing instances. Obviously, there would be redundant loops when one field has multiple instances in site. If we get only fields list in the very beginning, it could save a lot of headache, imho:)

StatusFileSize
new119 bytes
new7.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,684 pass(es).
[ View ]

I think we're fine with MENU_NORMAL_ITEM. I agree with @smiletrl regarding getFieldMap() and made the change.

Status:Needs review» Reviewed & tested by the community

Sweet

Status:Reviewed & tested by the community» Needs work

Hmm. Tests might pass, but I'm getting a large series of messages when I go to admin/reports/fields:

Notice: Undefined index: in Drupal\field_ui\Controller\FieldUIController->fieldsList() (line 86 of core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.php).
Notice: Undefined index: in Drupal\field_ui\Controller\FieldUIController->fieldsList() (line 87 of core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.php).
Notice: Undefined index: in Drupal\field_ui\Controller\FieldUIController->fieldsList() (line 87 of core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.php).
Notice: Trying to get property of non-object in Drupal\field_ui\Controller\FieldUIController->fieldsList() (line 87 of core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.php).
Notice: Undefined index: comment_body in Drupal\field_ui\Controller\FieldUIController->fieldsList() (line 92 of core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.php).
Warning: Invalid argument supplied for foreach() in Drupal\field_ui\Controller\FieldUIController->fieldsList() (line 79 of core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.php).

Right, sorry about that.

$this->fieldInfo->getInstances(); is fine - this also just an admin overview, we don't really care about performance here.

Issue tags:+Needs tests

Additionally, we do need al the instances, as we're printing the bundles where they are used.

Okay, I'm going to:

-    $instances = $this->fieldInfo->getFieldMap();
+    $instances = $this->fieldInfo->getInstances();

and add a test so we can detect if this page gets broken at some point in the future.

Assigned:Unassigned» mparker17

Assigned:mparker17» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs tests
StatusFileSize
new1.84 KB
new8.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,719 pass(es).
[ View ]

Okay, let's try this.

Status:Needs review» Reviewed & tested by the community

Great!

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new2.9 KB
new8.01 KB
PASSED: [[SimpleTest]]: [MySQL] 55,800 pass(es).
[ View ]

New patch for #19. This isn't a major/performance issue, but more of "which is the best practice" thing.

Personally speaking, I think
$fields = $this->fieldInfo->getFieldMap();
makes code cleaner/clearer and pretty straightforward~

StatusFileSize
new2.92 KB
new8.02 KB
PASSED: [[SimpleTest]]: [MySQL] 55,513 pass(es).
[ View ]

Oops, extral spaces...
new patch fix it.

Also, I think

+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('plugin.manager.entity'),
+      $container->get('field.info')
+    );
+  }

should be put before __construct, since method create will be executed firstly before _contruct?

Status:Needs review» Reviewed & tested by the community

That's fine. There have been a couple of other conversion which went in where the construct method is at the top. __construct methods are also usually at the top of the class anyway.

Tested manually and works fine and we have a test, sweet!

#30: 1987708-route-field-ui-30.patch queued for re-testing.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

curl https://drupal.org/files/1987708-route-field-ui-30.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8216  100  8216    0     0   7924      0  0:00:01  0:00:01 --:--:--  9575
error: patch failed: core/modules/field_ui/field_ui.admin.inc:6
error: core/modules/field_ui/field_ui.admin.inc: patch does not apply

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new7.96 KB
PASSED: [[SimpleTest]]: [MySQL] 55,062 pass(es).
[ View ]

Simple reroll after #1982138: Clean out field_ui.admin.inc got in.

Status:Reviewed & tested by the community» Needs work

Needs a reroll

curl https://drupal.org/files/1987708-route-field-ui-35.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  8153  100  8153    0     0    833      0  0:00:09  0:00:09 --:--:--  2038
error: patch failed: core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php:533
error: core/modules/field_ui/lib/Drupal/field_ui/Tests/ManageFieldsTest.php: patch does not apply

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new8.01 KB
FAILED: [[SimpleTest]]: [MySQL] 57,742 pass(es), 4 fail(s), and 1 exception(s).
[ View ]

Issue tags:-Needs reroll

Removing tg

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1987708-route-field-ui-37.patch, failed testing.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new742 bytes
new8.02 KB
PASSED: [[SimpleTest]]: [MySQL] 57,588 pass(es).
[ View ]

Oh ControllerInterface ...

Status:Reviewed & tested by the community» Needs work

I wonder if we can make this use EntityListController?

Noticed some minor nits...

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.phpundefined
@@ -0,0 +1,106 @@
+      $field = field_info_field($field_name);

This can use $this->fieldInfo->getField()

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Controller/FieldUIController.phpundefined
@@ -0,0 +1,106 @@
+      $output = theme('table', array('header' => $header, 'rows' => $rows));

I think this could just return an array like this..

array(
  '#theme' => 'table',
  '#header' => $header,
  '#rows' => $rows,
)

Status:Needs work» Needs review
StatusFileSize
new8.67 KB
new8.62 KB
PASSED: [[SimpleTest]]: [MySQL] 58,128 pass(es).
[ View ]

Yes, let's use the ListController.

Good work!

It's nice to replace the 0, 1, 2 keys with meaningful keys 'type', etc.

Status:Needs review» Reviewed & tested by the community

Sweet

Status:Reviewed & tested by the community» Fixed

Committed 8b1ce34 and pushed to 8.x. Thanks!

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