field_ui_next_destination() is only used by the "Manage Fields" forms, and they are similar enough to share a base class (added here).
field_ui_get_destinations() is used three times (once for no reason), but is not a Field UI-specific function, and should just be called inline as needed.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Status: Active » Needs review
FileSize
24.98 KB
andypost’s picture

suppose better to postpone this on #1982088: Remove hook_entity_bundle_info()'s need for 'real path'
+1 to RTBC

swentel’s picture

tim.plunkett’s picture

Title: Replace field_ui_get_destinations() and field_ui_next_destination() » Clean out field_ui.admin.inc
FileSize
19.01 KB

In light of http://buytaert.net/reducing-risk-in-the-drupal-8-release-schedule, let's stop tiptoeing around and just clean things up for real.

Each of the following are called only once, in either FieldOverview/DisplayOverview:
field_ui_field_type_options()
field_ui_formatter_options()
field_ui_existing_field_options()
field_ui_widget_type_options()

Any thoughts on converting to methods?

I'm going to open an issue about allowing any callable for machine_name 'exists', which kills _field_ui_field_name_exists()

field_ui_fields_list() is #1987708: Convert field_ui_fields_list() to a new style controller

_field_ui_reduce_order() is weird, whatever.

Then we are left with field_ui_table_pre_render() and theme_field_ui_table(). The latter is Twig fodder.

The reroll in #3 was only half of the original, and the rest conflicted, so no real interdiff.

Status: Needs review » Needs work

The last submitted patch, field-ui-1982138-4.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.29 KB
21.29 KB
swentel’s picture

Closed #1854868: Let formatter summaries return arrays instead of strings and #1854880: Cleanup DisplayOverview::form from our sandbox, maybe we can tackle that too at once.

The ultimate follow should be 'simply' renaming field ui to entity ui - although that might depend on #1875974: Abstract 'component type' specific code out of EntityDisplay (although it's not a requirement)

I will do those seperately

swentel’s picture

Addendum, I'll work on top of this one, depending on the size, I might split it anyway :)

swentel’s picture

The ultimate follow should be 'simply' renaming field ui to entity ui - although that might depend on #1875974: Abstract 'component type' specific code out of EntityDisplay (although it's not a requirement).

tim.plunkett’s picture

FileSize
18.26 KB
35.82 KB

Useful and generic functions:
field_ui_formatter_options() => FormatterPluginManager::getFormatterOptions()
field_ui_widget_type_options() => WidgetPluginManager::getWidgetOptions()

Specific functions:
field_ui_field_type_options() moved inline
field_ui_existing_field_options() => FieldOverview::getExistingFieldOptions()

Total removed code from field_ui.admin.inc: 211 lines, 9 functions
Remaining code in field_ui.admin.inc: 251 lines, 5 functions

Status: Needs review » Needs work

The last submitted patch, field-ui-1982138-10.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.59 KB
35.83 KB

Grr, FieldOverview uses $this->entity_type not $this->entityType.

Status: Needs review » Needs work

The last submitted patch, field-ui-1982138-12.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
2.68 KB
38.12 KB

Serious deja vu here.

tim.plunkett’s picture

yched’s picture

Overall this looks simply lovely !

A couple remarks :

+++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldInstanceFormBase.phpundefined
@@ -0,0 +1,92 @@
+  /**
+   * The field widget plugin manager.
+   *
+   * @var \Drupal\field\Plugin\Type\Widget\WidgetPluginManager
+   */
+  protected $widgetManager;

Seems a little weird to have the $widgetManager here - FieldEditForm, which extends this base class, doesn't need one.

This being said, FieldEditForm extends FieldInstanceFormBase is weird too :-).

TBH, aside from the getNextDestination() method, I'm not sure I see the real gain of the base FieldInstanceFormBase class and the other methods in there.
For example, I'd rather have the "$this->instance = $form_state['instance'] = $field_instance;" code explicitly repeated in each child buildForm() method, than mutualized / hidden away over here.

Then, if it's only to have a place for getNextDestination(), maybe it would make more sense as a method on a static FieldUI helper API class like we tend to introduce in core modules ?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -206,14 +240,31 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
+      // Skip field types which have no widget types, or should not be add via
+      // uesr interface.

Copy/pasted, but two typos here:
"be added" / "user interface" (would "via the UI" be acceptable ?)

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -206,14 +240,31 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
+    // Prepare the widget types to be display as options.
+    $widget_type_options = array();
+    foreach ($widget_options as $field_type => $widgets) {
+      $widget_type_options[$field_types[$field_type]['label']] = $widgets;
+    }

Typo in the comment here as well.
Also, this code should move a couple lines up, after the call to getWidgetOptions() (the code is "start X, do unrelated Y, finish X")

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -402,7 +453,7 @@ public function buildForm(array $form, array &$form_state, $entity_type = NULL,
+      'data' => array('fields' => $js_fields, 'fieldWidgetTypes' => $this->widgetManager->getWidgetOptions()),

Minor: this could reuse the $widget_options variable defined above.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/OverviewBase.phpundefined
@@ -130,4 +130,13 @@ public function getRegionOptions() {
+  /**
+   * Returns the region to which a row in the overview screen belongs.
+   *
+   * This function is to be used as a #region_callback in
+   * \Drupal\field_ui\Form\OverviewBase::form(), and is called during
+   * field_ui_table_pre_render().
+   */
+  abstract public function getRowRegion($row);

"in OverviewBase::form()" is not really true, OverviewBase doesn't refer to the getRowRegion() method (+ the actual method name is buildForm()).
DisplayOverview and FieldOverview just happen to both use such a method, but I'm not sure it deserves to have an abstract definition here ? You don't *need* to implement it to be allowed to extend OverviewBase ?

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -58,27 +58,6 @@ function field_ui_fields_list() {
 /**
- * Displays a message listing the inactive fields of a given bundle.
- */
-function field_ui_inactive_message($entity_type, $bundle) {

Hm, this function doesn't seem to be called anywhere in HEAD now, but it was called in both "field overview" and "display overview" before #1946404: Convert forms in field_ui.admin.inc to the new form interface - see http://drupalcode.org/project/drupal.git/commitdiff/6ed57d3.
I'm not sure that removing those calls was intentional ?
[edit: also, if we *do* remove that function, then field_ui_inactive_instances($entity_type, $bundle) can be deleted as well]

Other than that:
_field_ui_reduce_order() could be an inline closure within field_ui_table_pre_render(), I guess. No biggie either way.
field_ui_table_pre_render() itself could potentially be a helper method in OverviewBase, but drupal_render() currently doesn't allow methods as #pre_render callbacks.

yched’s picture

Also, looks like this is going to conflict with #1875992: Add EntityFormDisplay objects for entity forms

tim.plunkett’s picture

tim.plunkett’s picture

Status: Needs review » Postponed
tim.plunkett’s picture

Okay, so the FieldEditForm / getNextDestination bit is a good point. I have not yet done anything about that.

This still includes the RTBC callable patch.

andypost’s picture

Seems better to include #1998992: Typo in FieldOverview.php

jibran’s picture

Status: Postponed » Needs review

As per #18

Status: Needs review » Needs work

The last submitted patch, field-ui-1982138-20.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
48.3 KB

Straight reroll.

tim.plunkett’s picture

FileSize
5.59 KB
50.25 KB

Okay, here it is with the static helper.

amateescu’s picture

FileSize
7.05 KB
51.44 KB

I have only one small problem with this patch: I think the getFormatterOptions()/getWidgetOptions() methods should both be renamed to just getOptions() so they can be used generically by a (base) class that works with either formatter OR widget plugins (yes, I'm leveling the ground for the form modes patch :P).

Implemented this change in the patch attached, along with some other minor cleanups.

RTBC anyone? :)

Status: Needs review » Needs work

The last submitted patch, field-ui-1982138-26.patch, failed testing.

amateescu’s picture

Status: Needs work » Needs review
FileSize
567 bytes
51.39 KB

Pfff.

swentel’s picture

Looks good to me, I'll let yched do the final call.

Also, disabled modules is going to be removed anyway now, so we don't need that inactive message :D

tim.plunkett’s picture

Assigned: tim.plunkett » yched
FileSize
51.42 KB

Straight reroll.

yched’s picture

Two minor fixes:
- removes an unused variable
- adds a missing \ before Exception

Other than that, well, the fact that we add object methods as FAPI #callbacks means we're now serializing DisplayOverview / FieldOverview objects in $form, and we hit the serialization chain of hell mentioned in #2004282-2: Injected dependencies and serialization hell (serializing all of those objects injected dependencies as well - here, the widget / formatter managers and their list of cached definitions), with the subtle associated bugs down the road.

Strictly speaking, those methods are just a copy/paste of former functional helpers. They are just moved to methods for better code organization, but they don't rely on the state of the object - of course, since their former functional version didn't - that's how FAPI #callbacks worked in D7, all external state info needs to be placed in $element.
So these helpers could be made static methods, and added as '#callback' => array(array(get_class($this), 'methodName')). (OptionsWidgetBase does this to avoid unneeded serialization)

Not sure what to do with this. We have already started doing this in other places, so it should probably not be held against this specific patch...

yched’s picture

FileSize
1.45 KB
51.65 KB

Forgot to upload the minor changes mentioned in #31

swentel’s picture

Status: Needs review » Reviewed & tested by the community

Good to go.

alexpott’s picture

Title: Clean out field_ui.admin.inc » Change notice: Clean out field_ui.admin.inc
Priority: Normal » Critical
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Nice refactor!

Committed 7f99ae9 and pushed to 8.x. Thanks!

swentel’s picture

As far as I can see, this doesn't really need a change notice. The only useful ones /might/ be FormatterPluginManager::getOptions and WidgetPluginManager::getOptions - but I don't really think people use those, unless I'm completely wrong ?

andypost’s picture

The only useful ones /might/ be FormatterPluginManager::getOptions and WidgetPluginManager::getOptions - but I don't really think people use those

Suppose better to update existing change notice with latest changes

YesCT’s picture

@andypost OK, which change notice should we update?
I looked at the list of change records that have something about plugins: https://drupal.org/list-changes/drupal?keywords_description=Plugin&to_br...
but none really jumped out at me.

swentel’s picture

Status: Active » Needs review

Change notice added at https://drupal.org/node/2017627

amateescu’s picture

Title: Change notice: Clean out field_ui.admin.inc » Clean out field_ui.admin.inc
Priority: Critical » Normal
Status: Needs review » Fixed
Issue tags: -Needs change record

Yeah, I think that's all we need for this change.

Status: Fixed » Closed (fixed)

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

yched’s picture