Add IEF integration

Commerce price table should implement hook_inline_entity_form_entity_form_alter() to hide the price field on IEF forms.

I needed the same thing -- thanks Bojanz for the direction. Here's a patch.

Ran into #1341294: Integrity constraint violation: 1048 Column 'commerce_unit_price_currency_code' cannot be null as client started adding products with this fix in place (since the hidden field creates no price, and thus sets no currency formatter). Hopefully I'll be able to figure out how the default (non-IEF) implementation solves this.

I found the patch at #19 from #1341294: Integrity constraint violation: 1048 Column 'commerce_unit_price_currency_code' cannot be null needed for non-IEF (at least on my existing site), so I've added it to this patch. Unfortunately I don't have time to test out why some people are getting it to work without that patch.

New patch, because that's what I get for posting without testing! :)

I don't think we should place any formatting as default, won't '0' be just fine?

Sadly, I didn't saw this issue earlier.

So, what do you think about my solution?

This patch removes the commerce_price from the IEF form (if the setting has been enabled) and adds a custom implementation of CommerceProductInlineEntityFormController to display the table as an item list within the IEF table. For this, it introduces a new submodule called commerce_price_inline_entity_table.

Update 2013-01-18: Please ignore this patch, it does not work as intended.

I've reworked my patch from #9. It's now based on akalata's patch.

Explanation of the attached changes:

M commerce_price_table.module

Basically, this is akalata's patch. I've only de-duplicated some code by calling commerce_price_table_form_commerce_product_ui_product_form_alter() from commerce_price_table_inline_entity_form_entity_form_alter() and ensured that if commerce_price has been made unaccessible by another module it will not be made accessible again.

A commerce_price_inline_entity_table/commerce_price_inline_entity_table.info

This is a new module which adds a custom InlineEntityFormController based on CommerceProductInlineEntityFormController to remove the "Price" column from the inline entity table (if hidden) and add a column for the "Price Table".

A commerce_price_inline_entity_table/commerce_price_inline_entity_table.module

Attaches the controller and ensures that it's weight is higher that the original controller's weight. Also, implements a formatter that is used to render the price table as a list, so it can be reformatted using CSS.

A commerce_price_inline_entity_table/includes/commerce_price_inline_entity_table_commerce_product.inline_entity_form.inc

The custom controller.

Please note that line 63 in commerce_price_inline_entity_table/commerce_price_inline_entity_table.module reads:

$header = commerce_price_table_display_quantity_headers($item, @$items[$delta + 1]);

This is based on my patch from http://drupal.org/node/1875688, where commerce_price_table_display_quantity_headers() expects two arguments instead of one, but it will also work with the original code. PHP just ignores the undefined extra argument.

Thanks to all for the patches! generally looks great but this is really difficult to merge. The issue referenced in here is in need work, as far as I understand it.

Please keep patches independent, otherwise it will be impossible to merge them. This is an issue to integrate IEF into the price tables, I'm totally ok with providing a bridge module for this if necessary, but if it's not too much code/integration I'd rather keep it in the same module, the price table module is not that big.

Setting to need work until we get a standalone patch to test the integration.

Oh, no, it applies perfectly on an unmodified 1.1 version. It's only the second parameter in line 63 in commerce_price_inline_entity_table/commerce_price_inline_entity_table.module. It can be removed (although it won't raise any errors or warnings because PHP allows functions to get more arguments than they defined). But it is required if the other patch is merged.

Update: The reason for the extra module is that the same trick (or call it a hack) may be used by some other module, as it is proposed in the ief forum (http://drupal.org/node/1521274). There is no legal way at the moment to extend this table. So I think it absolutely makes sense to keep this feature in an optional submodule. Additionally, ief is not and should not be a dependency for commerce price table, but it must be present to calculate the correct weight for the custom controller.

If you want to split it, just apply the patch and delete the commerce_price_inline_entity_table subfolder :-)

Here's the same patch without the second parameter.

Thanks for the clarification, as I've mentioned above, I'm ok with having a submodule as we seem to have a good reason to.
Here's a code review for the patch in #14

+++ b/commerce_price_inline_entity_table/commerce_price_inline_entity_table.info	Fri Jan 18 19:43:54 2013 +0100
@@ -0,0 +1,18 @@
+; Information added by drupal.org packaging script on 2012-06-29

Packaging script info should not be included

+++ b/commerce_price_inline_entity_table/commerce_price_inline_entity_table.module	Fri Jan 18 19:43:54 2013 +0100
@@ -0,0 +1,74 @@
+  // we'll use our custom contoller
...
+    // We'll define a custom view mode for this entity

Comments should start with uppercase and end with a dot.

+++ b/commerce_price_inline_entity_table/commerce_price_inline_entity_table.module	Fri Jan 18 19:43:54 2013 +0100
@@ -0,0 +1,74 @@
+ * implement hook_install()

Implements, dot at the end

+++ b/commerce_price_table.module	Fri Jan 18 19:43:54 2013 +0100
@@ -515,13 +515,22 @@
+      $form['commerce_price']['#access'] = false;

How is this related with IEF?

+++ b/commerce_price_table.module	Fri Jan 18 19:43:54 2013 +0100
@@ -515,13 +515,22 @@
+function commerce_price_table_inline_entity_form_entity_form_alter(&$entity_form, $form_state) {

This should be in the submodule, right?

Thanks for the hints, I've removed the packaging info from the submodule and corrected the comments.

How is this related with IEF?

It isn't, I've removed it from the patch and will make a separate issue from it.

This should be in the submodule, right?

Since it doesn't make many sense to hide the default price field but keeping it in the IEF summary I think you're right. Changed it.

Back to CNR :)

Could you provide the patch from the module root?

The patch command has a parameter "-p1" that strips the first component from the path. Diffing in the same directory would mean to have an *.orig copy from each modified file. The way I (and most people I know) create a patch (using "diff -rNu ORIG_PATH NEW_PATH") ensures that no modified or created files are missed.

I'm talking about the root of the module, git diff is normally what I use for this kind of things.
There's extensive documentation about how this should be done: http://drupal.org/node/707484

I still don't understand what's the difference between

diff -rNu commerce_price_table-1.1/filename commerce_price_table/filename
--- commerce_price_table-1.1/filename
+++ commerce_price_table/filename

and

diff --git a/filename b/filename
--- a/filename
+++ b/filename

since both are applied the same way (patch -p1 < input.patch) and the diff's content is exactly the same.

I used the chance to rename the submodule. It's now called commerce_price_table_ief_support (Commerce Price Table IEF support).I still don

Needs review, again

Take a look to the patch in #1589950: Compact table & hide headers
diff -rNu commerce_price_table-1.1/commerce_price_inline_entity_table/commerce_price_inline_entity_table.info commerce_price_table/commerce_price_inline_entity_table/commerce_price_inline_entity_table.info

vs
diff --git a/commerce_price_table.module b/commerce_price_table.module

Patches in drupal issue queues should not assume any path, they should be generated from within the module root.

Edit: fixed the right patch example :P

Sorry didn't intend to change status, this patch is ok

Please name the module and the files the same way, I've got a folder called commerce_price_table_ief_support but the module is called commerce_price_intline_entity_table.module, that's confusing.

I'm fine with "commerce_price_table_ief"

I'm testing this and I can't find the formatter "commerce_multiprice_list_ief" anywhere in the ui and it's not displayed in the inline entity form.

I've renamed the module (and didn't forget the .info and .module file this time) and tested it.

The formatter is used within the IEF summary table, and can be selected as a Format in "Administration > Store > Configuration > Product variation types > Product Manage display > Price Table" (although this is not the intended use for it, but who knows, maybe someone likes it because it's easier to style a list with CSS than a table).

Maybe it's a conflict with the old module's name, if you have applied the "ief_integration-1823012-7-rt4.patch" patch previously. Can you try to restore the old submodule, uninstall it from the module settings page and than switch to the new submodule?

Oh, with this patch, the formatter appears in the admin displays for the products, it wasn't there before, not sure why.

Almost there!

+++ b/commerce_price_table_ief/commerce_price_table_ief.module
@@ -0,0 +1,82 @@
+ * Implement hook_install().

"Implements" and this should go in a .install file

+++ b/commerce_price_table_ief/commerce_price_table_ief.module
@@ -0,0 +1,82 @@
+/**
+ * Implements hook_inline_entity_form_entity_form_alter().
+ */
+function commerce_price_table_ief_inline_entity_form_entity_form_alter(&$entity_form, $form_state) {
+  commerce_price_table_form_commerce_product_ui_product_form_alter($entity_form, $form_state, 'commerce_product_ui_product_form');

Is this still needed even if you override the class?

+++ b/commerce_price_table_ief/includes/commerce_price_table_ief_commerce_product.inline_entity_form.inc
@@ -0,0 +1,30 @@
+<?php

We need a @file block commenting what is this file and a comment on what the class actually does

+++ b/commerce_price_table_ief/includes/commerce_price_table_ief_commerce_product.inline_entity_form.inc
@@ -0,0 +1,30 @@
+            $fields['commerce_price']['visible'] = false;

FALSE

+++ b/commerce_price_table_ief/includes/commerce_price_table_ief_commerce_product.inline_entity_form.inc
@@ -0,0 +1,30 @@
+      'weight' => @$fields['commerce_price']['weight'] + 1,

Why would you need a '@' here?

Is this still needed even if you override the class?

Yes. The class only changes the summary table, not the form. hook_inline_entity_form_entity_form_alter() is the IEF equivalent to hook_form_FORMNAME_alter() for the embedded product form.

Why would you need a '@' here?

To suppress a warning if the commerce_price field hasn't been returned by the superclass for some unknown reason, or has no weight defined. In this case, the expression simply evaluates to 1.

To suppress a warning if the commerce_price field hasn't been returned by the superclass for some unknown reason, or has no weight defined. In this case, the expression simply evaluates to 1.

Then just use isset or empty:

isset($fields['commerce_price']['weight']) ? $fields['commerce_price']['weight'] : 0

Using a ternary operator with isset() is ok here, just a longer expression with basically the same result (personally, I prefer "@$var" to "isset($var) ? $var : 0" for this reason wherever it makes sense). empty() won't work, because it would result in a weight of 0 instead of 1 if the original weight is 0 or empty.

I don't think @ is thought to replace isset: http://michelf.ca/blog/2005/bad-uses-of-the-at-operator/

I don't think @ is thought to replace isset: http://michelf.ca/blog/2005/bad-uses-of-the-at-operator/

Thanks, that wasn't clear to me until now. I did a benchmark and even had a factor 5 difference if the field doesn't exist (and a factor 2 difference if it exists). Very interesting, I was always sure that PHP uses a more efficient way to handle error suppressing.

Changes applied, plus one variable name change ($listItems -> $list_items).

Amazing work here, commited, thanks!!!