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.

Files: 
CommentFileSizeAuthor
#32 field-ui-1982138-31.patch51.65 KByched
PASSED: [[SimpleTest]]: [MySQL] 57,154 pass(es).
[ View ]
#32 interdiff.txt1.45 KByched
#30 field-ui-1982138-30.patch51.42 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,112 pass(es).
[ View ]
#28 field-ui-1982138-28.patch51.39 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 55,883 pass(es).
[ View ]
#28 interdiff.txt567 bytesamateescu
#26 field-ui-1982138-26.patch51.44 KBamateescu
FAILED: [[SimpleTest]]: [MySQL] 55,747 pass(es), 264 fail(s), and 635 exception(s).
[ View ]
#26 interdiff.txt7.05 KBamateescu
#25 field-ui-1982138-25.patch50.25 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,852 pass(es).
[ View ]
#25 interdiff.txt5.59 KBtim.plunkett
#24 field-ui-1982138-24.patch48.3 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,923 pass(es).
[ View ]
#20 field-ui-1982138-20.patch52 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-ui-1982138-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 interdiff.txt14.92 KBtim.plunkett
#14 field-ui-1982138-14.patch38.12 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,520 pass(es).
[ View ]
#14 interdiff.txt2.68 KBtim.plunkett
#12 field-ui-1982138-12.patch35.83 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,553 pass(es), 84 fail(s), and 89 exception(s).
[ View ]
#12 interdiff.txt1.59 KBtim.plunkett
#10 field-ui-1982138-10.patch35.82 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,431 pass(es), 80 fail(s), and 491 exception(s).
[ View ]
#10 interdiff.txt18.26 KBtim.plunkett
#6 field-ui-admin-1982138-6.patch21.29 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,578 pass(es).
[ View ]
#6 interdiff.txt2.29 KBtim.plunkett
#4 field-ui-1982138-4.patch19.01 KBtim.plunkett
FAILED: [[SimpleTest]]: [MySQL] 55,392 pass(es), 59 fail(s), and 33 exception(s).
[ View ]
#3 field-ui-1982138-3.patch9.1 KBswentel
PASSED: [[SimpleTest]]: [MySQL] 55,613 pass(es).
[ View ]
#1 field-ui-1982138-1.patch24.98 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 55,403 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new24.98 KB
PASSED: [[SimpleTest]]: [MySQL] 55,403 pass(es).
[ View ]

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

StatusFileSize
new9.1 KB
PASSED: [[SimpleTest]]: [MySQL] 55,613 pass(es).
[ View ]

Title:Replace field_ui_get_destinations() and field_ui_next_destination()Clean out field_ui.admin.inc
StatusFileSize
new19.01 KB
FAILED: [[SimpleTest]]: [MySQL] 55,392 pass(es), 59 fail(s), and 33 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.29 KB
new21.29 KB
PASSED: [[SimpleTest]]: [MySQL] 55,578 pass(es).
[ View ]

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

StatusFileSize
new18.26 KB
new35.82 KB
FAILED: [[SimpleTest]]: [MySQL] 55,431 pass(es), 80 fail(s), and 491 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new1.59 KB
new35.83 KB
FAILED: [[SimpleTest]]: [MySQL] 55,553 pass(es), 84 fail(s), and 89 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new2.68 KB
new38.12 KB
PASSED: [[SimpleTest]]: [MySQL] 55,520 pass(es).
[ View ]

Serious deja vu here.

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.

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

Status:Needs review» Postponed

StatusFileSize
new14.92 KB
new52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch field-ui-1982138-20.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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.

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

Status:Postponed» Needs review

As per #18

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new48.3 KB
PASSED: [[SimpleTest]]: [MySQL] 55,923 pass(es).
[ View ]

Straight reroll.

StatusFileSize
new5.59 KB
new50.25 KB
PASSED: [[SimpleTest]]: [MySQL] 55,852 pass(es).
[ View ]

Okay, here it is with the static helper.

StatusFileSize
new7.05 KB
new51.44 KB
FAILED: [[SimpleTest]]: [MySQL] 55,747 pass(es), 264 fail(s), and 635 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new567 bytes
new51.39 KB
PASSED: [[SimpleTest]]: [MySQL] 55,883 pass(es).
[ View ]

Pfff.

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

Assigned:tim.plunkett» yched
StatusFileSize
new51.42 KB
PASSED: [[SimpleTest]]: [MySQL] 56,112 pass(es).
[ View ]

Straight reroll.

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

StatusFileSize
new1.45 KB
new51.65 KB
PASSED: [[SimpleTest]]: [MySQL] 57,154 pass(es).
[ View ]

Forgot to upload the minor changes mentioned in #31

Status:Needs review» Reviewed & tested by the community

Good to go.

Title:Clean out field_ui.admin.incChange 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!

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 ?

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

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

Status:Active» Needs review

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

Title:Change notice: Clean out field_ui.admin.incClean 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.