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

vijaycs85’s picture

Title: Convert field_test_entity_add() to a new style controller » Convert field_ui_fields_list() to a new style controller
swentel’s picture

Assigned: Unassigned » swentel
Issue tags: +Field API
swentel’s picture

Status: Active » Needs review
FileSize
6.34 KB
FAILED: [[SimpleTest]]: [MySQL] 55,712 pass(es), 2 fail(s), and 0 exception(s). View
yched’s picture

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

:-)

manimejia’s picture

Status: Needs work » Needs review

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

swentel’s picture

Assigned: swentel » Unassigned

Unassigning for now

Status: Needs review » Needs work

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

mtift’s picture

Assigned: Unassigned » mtift

I'll take a crack at this one

mtift’s picture

Assigned: mtift » Unassigned
Status: Needs work » Reviewed & tested by the community
FileSize
2.13 KB
7.08 KB
PASSED: [[SimpleTest]]: [MySQL] 55,682 pass(es). View

The attached patch incorporates comments from #4

mtift’s picture

Status: Reviewed & tested by the community » Needs review

Whoops. Needs review.

mtift’s picture

FileSize
117 bytes
7.07 KB
PASSED: [[SimpleTest]]: [MySQL] 55,916 pass(es). View

Renamed the _construct object

mtift’s picture

FileSize
210 bytes
7.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.

swentel’s picture

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.

swentel’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
444 bytes
7.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,685 pass(es). View

syntax error fix...

smiletrl’s picture

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

mtift’s picture

FileSize
119 bytes
7.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.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Sweet

mparker17’s picture

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).
swentel’s picture

Right, sorry about that.

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

swentel’s picture

Issue tags: +Needs tests

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

mparker17’s picture

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.

mparker17’s picture

Assigned: Unassigned » mparker17
mparker17’s picture

Assigned: mparker17 » Unassigned
Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
1.84 KB
8.2 KB
PASSED: [[SimpleTest]]: [MySQL] 55,719 pass(es). View

Okay, let's try this.

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Great!

smiletrl’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
2.9 KB
8.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~

smiletrl’s picture

FileSize
2.92 KB
8.02 KB
PASSED: [[SimpleTest]]: [MySQL] 55,513 pass(es). View

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

smiletrl’s picture

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?

swentel’s picture

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!

alexpott’s picture

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

alexpott’s picture

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
swentel’s picture

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

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

alexpott’s picture

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
swentel’s picture

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

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.

swentel’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
742 bytes
8.02 KB
PASSED: [[SimpleTest]]: [MySQL] 57,588 pass(es). View

Oh ControllerInterface ...

alexpott’s picture

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,
)
tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
8.67 KB
8.62 KB
PASSED: [[SimpleTest]]: [MySQL] 58,128 pass(es). View

Yes, let's use the ListController.

smiletrl’s picture

Good work!

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

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Sweet

alexpott’s picture

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.