Refactor field_ui so common behavior for fields and display overview screens are extracted

Let's see what the bot says now.

Oh my, took the patch wrong

Fix last remaining tests.

Hmm no, #1303412: 'Manage display' : Cannot drag in or out of 'Hidden' got in, so this needs to be removed from the patch.

All I could find were some code-style and readability issues. The concept and the code look fine in general.

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -299,429 +301,16 @@ function theme_field_ui_table($variables) {
+  $fieldOverview = new FieldUIFieldOverview($entity_type, $bundle);

Normally, we are using non-camel-case variables.

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -896,377 +339,18 @@ function field_ui_field_overview_form_submit($form, &$form_state) {
+  $displayOverview = new FieldUIDisplayOverview($entity_type, $bundle, $view_mode);

Same.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIDisplayOverview.phpundefined
@@ -0,0 +1,480 @@
+   * Implements Drupal\field_ui\FieldUIOverviewInterface::getRegions()

Missing "."... Should be "Implements Drupal\field_ui\FieldUIOverviewInterface::getRegions()." Same for all the following method docblocks.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIDisplayOverview.phpundefined
@@ -0,0 +1,480 @@
+

Empty line for no reason.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIDisplayOverview.phpundefined
@@ -0,0 +1,480 @@
+    // field_group), the 'format type' selects can trigger a series of changes in

Exceeds 80 characters.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIDisplayOverview.phpundefined
@@ -0,0 +1,480 @@
+      // Retrieve the stored instance settings to merge with the incoming values.

Exceeds 80 charaters.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIFieldOverview.phpundefined
@@ -0,0 +1,625 @@
+        // Place the 'translatable' property as an explicit value so that contrib

Exceeds 80 characters.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewBase.phpundefined
@@ -0,0 +1,74 @@
+   *   (optional) The view mode for the entity which takes a string or "default".

Exceeds 80 characters.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewInterface.phpundefined
@@ -0,0 +1,80 @@
+   *   (optional) The view mode for the entity which takes a string or "default".

Exceeds 80 characters.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewBase.phpundefined
@@ -0,0 +1,74 @@
+   * Implements Drupal\field_ui\FieldUIOverviewInterface::__construct()

Missing "." after Implements ... (Here and everywhere else too).

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewBase.phpundefined
@@ -0,0 +1,74 @@
+   *   (optional) The view mode for the entity which takes a string or "default".

Comment too long :)

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewInterface.phpundefined
@@ -0,0 +1,80 @@
+   *   An renderable array.

"A renderable array"

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewInterface.phpundefined
@@ -0,0 +1,80 @@
+   *   (optional) The view mode for the entity which takes a string or "default".

Here too!

Reviewed this, and everything seems ok. I think this can go to rtbc after the comments from fubhy are applied.

Patch size mostly caused by moving a big form code within files. I've done a manual testing - it works!
I think it RTBC

+++ b/core/modules/field_ui/field_ui.js
@@ -326,7 +326,9 @@ Drupal.fieldUIDisplayOverview.field.prototype = {
     var currentValue = this.$formatSelect.val();
     var value;
-    if (region === 'visible') {
+    // @TODO Check if this couldn't just be like
+    // if (region !== 'hidden') {
+    if (region === 'content') {
       if (currentValue === 'hidden') {

Better to use !== hidden (I've tested this change manually). If there could be more regions name... maybe this js could be re-used!

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewBase.php
@@ -0,0 +1,75 @@
+  public function getRegionOptions() {
+    $options = array();
+    foreach ($this->getRegions() as $region => $data) {
+      $options[$region] = $data['title'];
+    }
+    return $options;

Patch allows to use a different region-set but there's a lot of hard-coded references to 'content' and 'hidden'. Maybe better to always have this "key-regions" by default?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIFieldOverview.php
@@ -0,0 +1,623 @@
+  /**
+   * Validates the 're-use existing field' row of
+   * FieldUIFieldOverview::validateAddExisting().
+   *
+   * @see Drupal\field_ui\FieldUIOverviewInterface::validate()
+   */
+  private function validateAddExisting($form, &$form_state) {

This should be one-line, but let's leave it for follow-up with clean-up

Reviewed and approved :)

Great work !
So as I said earlier, I generally +1 on the principle. I have a couple remarks on the implementation though - nothing serious.

- Aside from the code reorganisation, it's difficult to see the changes within the code that gets moved around (e.g. the actual content of FieldUIDisplayOverview::form() vs the former field_ui_display_overview_form()). Are there any, and if so, can you summarize them ?

- We don't need the FieldUI prefix in the class names. The namespace is specific enough.

- Too bad we still need to keep field_ui_display_overview_multistep_js() in the .module file
Have you checked whether field_ui_display_overview_multistep_submit() could work as a class method, though ?

+++ b/core/modules/field_ui/field_ui.admin.inc
@@ -299,429 +301,16 @@ function theme_field_ui_table($variables) {
-function field_ui_field_overview_form($form, &$form_state, $entity_type, $bundle) {

field_ui_field_overview_form() and field_ui_display_overview_form() are not form callbacks anymore but regular page callbacks, that happen to return a rendered form. They should thus be renamed, to e.g. field_ui_field_overview(), field_ui_display_overview(), and their phpdoc be adjusted. See the phpdoc of entity_get_form() for an example.

When renaming, beware of code comments that still refer to the old form callbacks ("Form submission handler for buttons in field_ui_display_overview_form()", Handles multistep buttons in field_ui_display_overview_form()", "This function is used as a #region_callback in field_ui_field_overview_form()")

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIDisplayOverview.php
@@ -0,0 +1,480 @@
+   * Get the regions needed to create the overview form.

Not needed - 'implements' or 'overrides' is enough. Same applies to the other methods, unless there is something really specific to that given implementation you want to emphasize

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIDisplayOverview.php
@@ -0,0 +1,480 @@
+  /**
+   * Implements Drupal\field_ui\FieldUIOverviewInterface::form().
+   *
+   * Create a renderable form.
+   */
+  public function form($form, &$form_state) {

Lacks type hinting in the signature (core type hints 'array' for arrays and class name for objects) and a proper docblock with @param and @return statements - same applies to other methods.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIDisplayOverview.php
@@ -0,0 +1,480 @@
+    // Add the validate and submit behavior.
+    $form['#validate'] = array(array($this, 'validate'));
+    $form['#submit'] = array(array($this, 'submit'));

Minor, but looks like this could be moved to FieldUIOverviewBase::form() ?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIDisplayOverview.php
@@ -0,0 +1,480 @@
+    $form += array(
+      '#entity_type' => $this->entity_type,
+      '#bundle' => $this->bundle,
+      '#view_mode' => $this->view_mode,
+      '#fields' => array_keys($instances),
+      '#extra' => array_keys($extra_fields),
+    );

Let's move those to $form_state (probbly best in a followup)

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIFieldOverview.php
@@ -0,0 +1,623 @@
+        'message' => t('No unused fields.'),

Not clear to me when this message would appear, but the wording seems weird.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIFieldOverview.php
@@ -0,0 +1,623 @@
+  private function validateAddNew($form, &$form_state) {

Private methods are frowned upon, needlessly restrictive - 'protected' is sufficient. Same applies to validateAddExisting()

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewBase.php
@@ -0,0 +1,75 @@
+  protected $entity_type = '';
+  protected $bundle = '';
+  protected $view_mode = '';
+  protected $adminPath = NULL;

The properties should have a phpdoc block with a sentence and a @var statement.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewBase.php
@@ -0,0 +1,75 @@
+   * Get all regions as options.

Should be 3rd person : "Gets" - I didn't closely check the rest of the methods on this aspect.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewBase.php
@@ -0,0 +1,75 @@
+    if (!isset($this->adminPath)) {
+      $this->adminPath = _field_ui_bundle_admin_path($this->entity_type, $this->bundle);
+    }

"statically" caching seems overkill. I'm not sure we need a property for that, just proxy to the function.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewBase.php
@@ -0,0 +1,75 @@
+  public function validate($form, &$form_state) {
+    // Nothing to do.
+  }

Seems not too consistent to define an empty validate() here and not an empty submit().
I get that FieldUIDisplayOverview has no validation, but I'd rather see none in the Base and have both subclasses add explicit methods (explixitely empty for FieldUIDisplayOverview)

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewInterface.php
@@ -0,0 +1,81 @@
+ * Defines a common interface for field overview UI's.

Language - "for Field UI overview pages" ?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewInterface.php
@@ -0,0 +1,81 @@
+interface FieldUIOverviewInterface {

No strong opinion here, but I don't really get why we need an interface. A base class, yes, but an interface ? Do we expect to have generic code that works on whatever FieldUIOverviewInterface object ?

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewInterface.php
@@ -0,0 +1,81 @@
+  public function __construct($entity_type, $bundle, $view_mode = 'default');

We usually try to leave _construct() method out of interfaces. Different child classes might need different construction logic / params.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldUIOverviewInterface.php
@@ -0,0 +1,81 @@
+  public function getRegions();
+}

Misses an empty line after the last method - same applies to the actual class files

29 days to next Drupal core point release.

Thanks for the changes, @Stalski - not easy for me to review code now, so I'll leave it to others to drive this home.

re: adminPath()

In fact, before it was a variable used in the complete form api code. So I tried to just do the same thing as before, which is a property.

Yeah. Well then, why don't we just populate it in the construct and be done with it ?

re: interface :

that's in fact especially for the getRegions method.

Then maybe just put getRegions() in the base class as an abstract method and ditch the interface ?

This looks good to me. The question is, should we wait for #1802072: Ajax callbacks should be callbacks defined as function or class method to get in first, or go ahead ? Could be done in a follow up of course. If anyone else can review the last one and think it's good, rtbc it.

@Stalski please update issue summary from your last comments, and I think it's ready.

Actually there's big changes depends on this issue so let's get this commited and focus on
#1770720: [META] Gradual changes to Field UI
#1786198: Make consistent regions in code for fields UI overview screens

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -290,438 +292,32 @@ function theme_field_ui_table($variables) {
+  $field_overview = new FieldOverview($entity_type, $bundle);

@@ -743,532 +339,38 @@ function _field_ui_field_name_exists($value) {
+  $display_overview = new DisplayOverview($entity_type, $bundle, $view_mode);

If these are hardcoded, they cannot be subclassed.

Thanks, I feel better already :)
+1 for RTBC

Doesn't apply anymore anyways. Doing a re-roll!

Something went wrong when #1802072: Ajax callbacks should be callbacks defined as function or class method should've been commited/pushed. Anyways. I re-rolled this one (didn't apply anymore). And created two patches for now (one, that incorporates the Ajax method callback patch for testing and one that doesn't [which is going to fail]).

Whoops, accidentally I killed the changes to operations introduced by #1480854: Convert operation links to '#type' => 'operations'. Re-adding those in this patch and also removing a few lines of stale (unused vars) code that I found while doing so.

Too impatient.

So the fact that the patch in #41 passes the tests maybe proofs that we need more field ui tests. It shouldn't pass considering that it uses the new ajax OO method callback from #1802072: Ajax callbacks should be callbacks defined as function or class method which hasn't been pushed yet.

+++ b/core/modules/field_ui/field_ui.admin.incundefined
@@ -290,437 +292,33 @@ function theme_field_ui_table($variables) {
 /**
- * Form constructor for the 'Manage fields' form of a bundle.
- *
+ * Returns the built and processed 'Manage fields' form of a bundle.
  * The resulting form allows fields and pseudo-fields to be re-ordered.

@@ -742,532 +340,38 @@ function _field_ui_field_name_exists($value) {
 /**
- * Validates the 're-use existing field' row of field_ui_field_overview_form().
+ * Returns the built and processed 'Manage display' form of a bundle.
+ * The resulting form allows fields and pseudo-fields to be re-ordered.

+++ b/core/modules/field_ui/lib/Drupal/field_ui/FieldOverview.phpundefined
@@ -0,0 +1,642 @@
+  /**
+   * Validates the 're-use existing field' row of
+   * FieldOverview::validateAddExisting().
+   *

Please format this as one-line summary according http://drupal.org/node/1354#functions

Another re-roll due to the recently pushed field formatter plugin-ification patch. Also incorporates the changes suggested by @andypost. The #ajax method callback patch is in now as well and also incorporated in this re-roll.

Let this be the last re-roll please :).

OK this has been RTBC for a while and was just stuck in re-roll hell.

I reviewed the patch, and while I'm not entirely comforable with the form callback as class model one-offs we're introducing for entities, it's probably as close as we can get for re-using that code in Drupal 8, so committed/pushed to 8.x. Hopefully this will unblock some more work.

Found a minor bug in this patch: @see #1824180: Undefined variable $admin_path in Field UI module