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
StatusFileSize
new6.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
StatusFileSize
new2.13 KB
new7.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

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

Renamed the _construct object

mtift’s picture

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.

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
StatusFileSize
new444 bytes
new7.13 KB
PASSED: [[SimpleTest]]: [MySQL] 55,685 pass(es).
[ View ]

syntax error fix...

smiletrl’s picture

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

mtift’s picture

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.

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
StatusFileSize
new1.84 KB
new8.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
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~

smiletrl’s picture

StatusFileSize
new2.92 KB
new8.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
StatusFileSize
new7.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
StatusFileSize
new8.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
StatusFileSize
new742 bytes
new8.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
StatusFileSize
new8.67 KB
new8.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.