Currently, the entity type specific code is placed in a separate file, and divided into a number of callbacks.
That file, and those callbacks are defined to IEF through hook_inline_entity_type_info().
Problem is, we're adding more and more callbacks, and many are optional. Also, different callbacks are optional in different use cases.
For example, the new commerce_discount module provides IEF integration, but is only interested in #1598296: Add a simplified widget for fields that accept only one entity of a specific type so it doesn't care about default fields or the delete form.
Also, #1527882: Handle deletion of non-deletable products requires a new callback that wraps entity_delete() so that the product integration can have specific logic.
Also found the same need for saving, since that's where the title autogenerating needs to happen (submit is the wrong place for it).
The list goes on and on.

joachim suggested investigating a controller model, so I did, and I'm very happy with the results.

1) Killed hook_inline_entity_type_info(), IEF just looks for a controller class defined in hook_entity_info().
2) Entity type specific logic is in controllers, each one inheriting from the base EntityInlineEntityFormController.
The base controller has enough logic that you can just override entityForm() and be done, which makes writing
integration much easier, and is a big step towards generic entity integration.

Things implemented / gained in the process:
1) The "product variation" language is now optional, there's a setting that the admin can set and decide between Product and Variation, fixing #1544276: Remove 'variation' from descriptions.
2) #1527882: Handle deletion of non-deletable products is now really easy to fix, posting a patch there.
3) Fixed title auto-generation. It was being done on entity form submit, which was too early (if the user creates an entity, and THEN adds or changes the node title, that change doesn't get reflected on the entity). Now moved to presave, works nicely.
4) Fixed UI problem (or at least made less bad) where SKU / Title columns where empty before auto-generation happened. Now shows a message saying "Will be auto-generated when the form is submitted".

Should also be really easy to implement an "unlink" setting (or any other) for all entity types, since settings have inheritance as well.

The patch is a bit big and hard to review. My suggestion is to apply it, and then open the three classes in includes/.
I would like to hear any comments on the new state of things, as well as any API nitpicks or suggestions. Now's the time!

CommentFileSizeAuthor
#13 1626706-ief-controllers.patch77.8 KBbojanz
#13 1626706-ief-controllers-interdiff.patch7.61 KBbojanz
#10 1626706-ief-controllers.patch79.44 KBbojanz
#9 1626706-ief-controllers.patch79.44 KBbojanz
#8 1626706-ief-controllers.patch51.16 KBbojanz
#1 1626706-ief-controllers.patch78.26 KBbojanz

Comments

bojanz’s picture

bojanz commented
Status: Active » Needs review
StatusFileSize
new 1626706-ief-controllers.patch78.26 KB
joachim’s picture

joachim commented

Wow!

I'm afraid I don't have time to look at this today, just wanted to say I was just thinking about this over the weekend and wondering whether the class hierarchy should be something like:

- base
-- class for some entity
-- sub-base for entities using EntityAPI (as they do lots of standard things)
--- class for an EntityAPI entity

I'll have a look at the patch tomorrow :)

bojanz’s picture

bojanz commented

We always rely on the Entity API in some extent, entity_save and entity_delete, for example.
The dividing point is Entity Metadata (which can pretty much only be used in entityForm / entityFormValidate / entityFormSubmit).
So, I agree that if we write a class that provides a form based on entity metadata (like VBO does for "modify entity values"), it should definitely be a separate class.

The question then is whether those entityForm methods should be empty in the base class. The submit method is generic enough to work for subclasses, but the other two are almost example only (however, makes no sense to only have submit...).

bojanz’s picture

bojanz commented
Priority: Normal » Critical

Updating priority. This blocks all other work (and pretty much breaks all other people's code :P)

Created another followup that addresses some more IEF feedback: #1627078: Simplify form state handling, remove $parents_key, please take a look.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
commented

I'm no expert in IEF, but moving to OOP makes a lot of sense.

+++ b/includes/commerce_line_item.inline_entity_form.inc
@@ -0,0 +1,136 @@
+  public function defaultFields($bundles) {

+++ b/includes/entity.inline_entity_form.inc
@@ -0,0 +1,279 @@
+  public function defaultFields($bundles) {

Shouldn't it get a single bundle by now? Getting fields by multiple Bundles sounds wrong.

+++ b/includes/entity.inline_entity_form.inc
@@ -0,0 +1,279 @@
+  /**
+   * Returns an array of css filepaths for the current entity type, keyed
+   * by theme name.
+   *
+   * If provided, the "base" CSS file is included for all themes.
+   * If a CSS file matching the current theme exists, it will also be included.
+   *
+   * @code
+   * return array(
+   *   'base' => drupal_get_path('module', 'test_module') . '/css/inline_entity_form.base.css',
+   *   'seven' => drupal_get_path('module', 'test_module') . '/css/inline_entity_form.seven.css',
+   * );
+   * @endcode
+   */
+  public function css() {
+    return array();
+  }

It's a nice utility function, but I don't think we need it. If anyway a user should override a function I think entityForm() is the right one. Otherwise we might need also to add a js() function

+++ b/includes/entity.inline_entity_form.inc
@@ -0,0 +1,279 @@
+  protected function cleanupFieldFormState($entity_form, $form_state) {
+    list(, , $bundle) = entity_extract_ids($entity_form['#entity_type'], $entity_form['#entity']);
+    $instances = field_info_instances($entity_form['#entity_type'], $bundle);
+    foreach ($instances as $instance) {
+      $field_name = $instance['field_name'];
+      $parents = $entity_form[$field_name]['#parents'];
+      array_pop($parents);
+      $langcode = $entity_form[$field_name]['#language'];
+
+      $field_state = field_form_get_state($parents, $field_name, $langcode, $form_state);
+      unset($field_state['items']);
+      $field_state['items_count'] = 0;
+      field_form_set_state($parents, $field_name, $langcode, $form_state, $field_state);
+    }
+  }

That's the reason I hate formAPI ;)

Might be a stupid question, but don't we want to actually drupal_rebuild_form()?

bojanz’s picture

bojanz commented

As discussed on IRC, the point of the css() method is to provide different available CSS files (base + per-theme files).
Killing the method and moving the CSS to an #attached inside entityForm() would mean that entityForm() would need to lookup the current theme and decide which files to add. That's slightly less convenient than the current approach.

Amitai argued that IEF shouldn't even handle per-theme CSS files, though I consider it a nice touch.

What do you guys think?

Shouldn't it get a single bundle by now? Getting fields by multiple Bundles sounds wrong.

No, this is intentional. $bundles are all allowed bundles for the current reference field (so you can choose to display only fields common to all bundles, etc). Obviously needs better docs then.

Might be a stupid question, but don't we want to actually drupal_rebuild_form()?

I don't think so.

joachim’s picture

joachim commented

I'm getting a pretty big reject file trying to apply this against the current HEAD.

bojanz’s picture

bojanz commented
StatusFileSize
new 1626706-ief-controllers.patch51.16 KB

EDIT: Wrong file uploaded.

bojanz’s picture

bojanz commented
StatusFileSize
new 1626706-ief-controllers.patch79.44 KB

Here's a reroll.

bojanz’s picture

bojanz commented
StatusFileSize
new 1626706-ief-controllers.patch79.44 KB

This one should apply.

andyg5000’s picture

andyg5000
Primary language English
Location North Carolina, USA
commented

I agree that moving to controllers w/ inheritance greatly increases the ability to scale this module out to meet all sorts of needs. In reviewing the code I found the following (some of this may be jumping the gun):

I attempted to use the entity reference module with IEF to reference commerce customer profile (or any other entity) on a node. The IEF form wasn’t added to the node edit form until I used hook_entity_info_alter() to add a controller for the reference type that I was using. I understand why, but wonder if the default entity controller should be assigned for all entity reference types unless altered.

Moving forward, I also tried to conceptualize how I would extend IEF in my own module for a custom entity reference. The ability to add controllers via hook_entity_info_alter() is obvious and the ability to use hook_field_wiget_info_alter() to add a new reference type to ‘field types’ is as well. However, adding to the $settings returned form inline_entity_form_settings() is not obvious. When I asked @bojanz about this in IRC he said I would have to create a patch for IEF to do this. It seems that moving to inherited controllers is a step towards making this a framework to build IEF references widgets from, but this seems like a hurdle to me.

The code looks good and is much easier to understand and build on. Thanks @bojanz!

bojanz’s picture

bojanz commented

I attempted to use the entity reference module with IEF to reference commerce customer profile (or any other entity) on a node. The IEF form wasn’t added to the node edit form until I used hook_entity_info_alter() to add a controller for the reference type that I was using. I understand why, but wonder if the default entity controller should be assigned for all entity reference types unless altered.

No, it shouldn't. The base controller doesn't have a complete entityForm method, so it can't be used out of the box (yet) to edit an unknown entity type, since the entity might be saved with required properties missing. The right workflow is overriding the base controller, then overriding the entityForm() method. I will document this on d.o.

Moving forward, I also tried to conceptualize how I would extend IEF in my own module for a custom entity reference. The ability to add controllers via hook_entity_info_alter() is obvious and the ability to use hook_field_wiget_info_alter() to add a new reference type to ‘field types’ is as well. However, adding to the $settings returned form inline_entity_form_settings() is not obvious. When I asked @bojanz about this in IRC he said I would have to create a patch for IEF to do this. It seems that moving to inherited controllers is a step towards making this a framework to build IEF references widgets from, but this seems like a hurdle to me.

My comment on IRC was that there's no point in ever writing a custom entityreference field now that we have entityreference itself, which supports plugins and can be extended in any needed way.

bojanz’s picture

bojanz commented
Status: Needs review » Fixed
StatusFileSize
new 1626706-ief-controllers-interdiff.patch7.61 KB
new 1626706-ief-controllers.patch77.8 KB

Ryan spent a large portion of yesterday reviewing and testing this code, and his reaction was favorable.

Committed a slightly tweaked version, full patch and interdiff attached.

I removed the "variation" language override from products, I will add that to the commerce_backoffice module which provides the same language for the backend views (and doing it by altering the "permission labels" will change the language in permissions as well, which is nice and consistent).
I also replaced the strings() method with a labels() method that just returns the singular and plural name of an entity type, so no hardcoded strings, which is nice.

Note that it is never too late for a followup, so feel free to continue reviewing and commenting.

EDIT: Committed the variation language override in a followup, since we decided to keep it in Inline Entity Form after all.

Status: Fixed » Closed (fixed)

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