The default rule shipped with the module assumes a certain field name. But if you name your field differently, it will fail. So, instead of using a default rule, we could just generate the right rule when the field is created and remove it when it is deleted?

For that we can use the module name as machine-name prefix instead of the regular 'rules_' prefix - which is for rules created via the admin UI. That way we can make sure we do not run into any conflicts.

Comments

You are right, I did not think of this when I've created the patch for #1875688: Expose price table to rules. The old implementation just looped through all price table fields, which lead to the situation that you could not have different price tables for different kind of customers.

The idea to add or remove the rule when the field is added or removed sounds right, since the rule would be useless without a price table field. On the other hand, if the user modified the rule and accidentally removed the field, he would to apply his changes again. But in this case he would also have to enter all price table items again, which would make much more effort.

Do you know how to add a rule configuration at runtime without using hook_default_rules_configuration()? I've found rules_config_delete() and rules_config_load(), but no rules_config_add() or something like that. Only rules_import(), which expects the rule configuration as serialized JSON data.

This would also be a fix of #1921668: Support Multiple Price Tables if my assumption made in comment #7 is right.

Assigned:Unassigned» Cybso

Working on it. Will require to set the field's cardinality to > 1 per default. Not sure how to treat existing rule(s).

Status:Active» Needs review
StatusFileSize
new2.85 KB

Implemented hook_field_create_field($field) and hook_field_delete_field($field) to create and delete individual rules called "commerce_price_table_{$fieldname}_override_price" for new price table fields that are not named field_price_table.

Thanks for tackling this :)
Comments above

+++ b/commerce_price_table.module
@@ -570,3 +570,76 @@ function commerce_price_table_display_quantity_headers($item) {
+    // The field cardinality *must* be > 1 for this module to make sense
+    // (and for the rule to work).
+    $field['cardinality'] = FIELD_CARDINALITY_UNLIMITED;
+    field_update_field($field);

I don't see a reason to do this, it might make sense to have price tables with 1 value, why not?
In any case this must be by definition and we should not mess with configurations this way.

+++ b/commerce_price_table.module
@@ -570,3 +570,76 @@ function commerce_price_table_display_quantity_headers($item) {
+    // Ensure that no rule with the desired name exists.
+    if (rules_config_load($rulename)) {
+      return;
+    }

If this might happen, why not ensure an unique rule machine name?

+++ b/commerce_price_table.module
@@ -570,3 +570,76 @@ function commerce_price_table_display_quantity_headers($item) {
+    if ($fieldname == 'field_price_table') {

field type I guess?

+++ b/commerce_price_table.module
@@ -570,3 +570,76 @@ function commerce_price_table_display_quantity_headers($item) {
+        'price_table:select' => 'commerce-line-item:commerce-product:'.str_replace('_', '-', $fieldname),

nitpick strtr is faster than str_replace for this small operations

dreditor missed one thing, "Implements hook_X" comment is missing for create and delete field.

I don't see a reason to do this, it might make sense to have price tables with 1 value, why not?

Because the rule expects a List<> field and won't work otherwise. Maybe it's possible to redefine the action parameters to accept multiple field types, I'll check this.

In any case this must be by definition and we should not mess with configurations this way.

According to http://api.drupal.org/api/drupal/modules!field!field.api.php/function/ho... the default cardinality cannot be defined in hook_field_info. Using hook_field_create_field() is the workaround recommend in the comments. (btw, it's only the default value, the form to select the cardinality is shown *after* hook_field_create_field has been displayed so the user is still free in his choice. He'll just get a default selection more reasonable in most situations).

If this might happen, why not ensure an unique rule machine name?

I think the probability that somebody named an existing rule this way is extremely low. Just ensure that the hook doesn't raise an error. But a loop to append _N in this case would be reasonable, too. (Edit: This would it make nearly impossible to automatically remove the rule in hook_field_delete_field without adding a new table or an additional setting to commerce_price_table which refers to the generated rule).

field type I guess?

No, field name is right. It's for the default rule, which refers to field_price_table by default. I've tried to remove the default rule, but I wasn't able to write an update method that replaces it by a custom rule for existing fields in reasonable time. It's easy when the rule hasn't been overridden, but I got problems to convert a customized rule into a custom rule. So I decided that it is OK for me to leave the original rule in place and just add custom rules if somebody chooses another name than field_price_table ;-)

nitpick strtr is faster than str_replace for this small operations

dreditor missed one thing, "Implements hook_X" comment is missing for create and delete field.

Thx, I'll fix this and provide an updated patch by tomorrow or so.

Status:Needs review» Needs work

According to http://api.drupal.org/api/drupal/modules!field!field.api.php/function/ho... the default cardinality cannot be defined in hook_field_info. Using hook_field_create_field() is the workaround recommend in the comments. (btw, it's only the default value, the form to select the cardinality is shown *after* hook_field_create_field has been displayed so the user is still free in his choice. He'll just get a default selection more reasonable in most situations).

I didn't mean in the info, but that part belongs to the create field itself and not the rule generation in case we want to add it.

No, field name is right. It's for the default rule, which refers to field_price_table by default. I've tried to remove the default rule, but I wasn't able to write an update method that replaces it by a custom rule for existing fields in reasonable time. It's easy when the rule hasn't been overridden, but I got problems to convert a customized rule into a custom rule. So I decided that it is OK for me to leave the original rule in place and just add custom rules if somebody chooses another name than field_price_table ;-)

If a rule has been overridden there's nothing the module can or even should do, again, we don't want to mess with user's configuration that way.
I thought we were dealing with creating default rules for all price tables here, if we're going to change a name dependency for another one, there's no point in this patch imho.

For ensuring an unique rule name, we can refer to fago's original comment:

For that we can use the module name as machine-name prefix instead of the regular 'rules_' prefix - which is for rules created via the admin UI. That way we can make sure we do not run into any conflicts.

Setting to NW.

What do you think about this?

1. Rewrite commerce_price_table_default_rules_configuration() so that it searches for all commerce_price_table fields and returns one rule for each field.

2. In hook_field_create_field and hook_field_delete_field, force drupal to refresh the default rules configuration.

This way there shouldn't be any naming conflicts since rules differs between custom and default rules, there is no need to remove the rule if the field is deleted, and the user can still customize the rule and even revert the customizations.

Same problem applies to ief integration. An equivalent solution as suggested in my previous comment for PriceTableCommerceProductInlineEntityFormController should fix this.

I guess that the best way to accomplish this is to loop the fields in hook_default_rules_configuration and generate a rule per table field.

That would drive us to an interesting situation, if you have several fields attached to a same entity type they will end up with several price replacing rules so we could just generate one with the highest price weight, not sure if makes a lot of sense to generate several rules by default that are going to step on top of each other.

We could create a separate issue to manage the unlimited setting by default to price tables.

Any fix for this? I tried using the dev version because of some issues with the other one but this one defines a buggy rule requiring 'commerce-line-item:commerce-product:field-price-table' when even commerce-product isn't available to the rule, let alone the field, and thus it doesn't work once I have created the field with the same name and tried updating the rule with the default selector that had earlier failed.

If I go back to previous rule definition from stable release, if I add a product wheres its quantity doesn't fall in any defined bracket, it picks up the wrong price durigg checkout instead of picking the base price.

I've reserved some time this weekend to fix this

For the wrong price replacement bug in case price falls outside defined quantity brackets, in the method 'commerce_price_table_get_amount_qty' in 'commerce_price_table.module', I replaced:

// Handle the unlimited qty.
foreach ($items as $item) {
     if ($item['max_qty'] == -1) {
       return $item;
     }
}
// We fallback to the higher one if no match was found.
return end($items);

with

// Handle the unlimited qty.
  foreach ($items as $item) {
    if ($quantity >= $item['min_qty'] && $item['max_qty'] == -1) {
      return $item;
    }
  }
// We fallback to the higher one if no match was found.
  return false;

But this should also take into account if the actual/base price is disabled, which it doesn't for now.

Since this is a totally different issue, can you create a separate ticket for this and add some additional details to it? For example, the output of "debug($items);" of this article?

Opened a separate issue: http://drupal.org/node/1980404

Status:Needs work» Needs review
StatusFileSize
new4.6 KB

New patch that generates dynamic default rules configurations for each field.

Change: Clear rules' cache only on field operations of type 'commerce_price_table'