Convert field_ui_fields_list() to a new style controller

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

:-)

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

Unassigning for now

I'll take a crack at this one

The attached patch incorporates comments from #4

Whoops. Needs review.

Renamed the _construct object

Added a missing param thanks to @swentel

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

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

To address concern for #4,

  $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:)

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

Sweet

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.

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.

Okay, let's try this.

Great!

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~

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?

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.

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

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

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

reroll after #1798654: Clean faulty HTML in help description of field widgets got in

Removing tg

Oh ControllerInterface ...

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

Yes, let's use the ListController.

Good work!

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

Sweet

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