Support from Acquia helps fund testing for Drupal Acquia logo

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

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

Renamed the _construct object

mtift’s picture

FileSize
210 bytes
7.13 KB

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

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

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

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

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

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

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

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

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.