Tax can create a rule when the rule already exists

Interesting problem... it strikes me that because we're just defining a default rule configuration, this shouldn't conflict with pre-existing rules. Is the problem that you have another module defining a default rule configuration with the same name? I wonder if there's a fix to be supplied upstream for this. : ?

I presume when you exported the tax rate to a Rule it implemented hook_commerce_tax_rate_info()? If that's the case, then it can specify 'rules_component' => FALSE telling the core Tax module to not define one. That's what the if ($tax_rate['rules_component']) { statement is all about.

This should be sufficient to resolve the issue here. I still wonder if there's a larger issue to address within Rules itself in the event that there is an unavoidable collision. What do you think? Good enough to close this particular issue for now?

Right, if rules_component is FALSE, then the Tax module itself won't attempt to define one using that naming convention. However, one thing I see that works against us is that the action to calculate taxes of a particular type also checks this boolean. This means right now you wouldn't be able to depend on the generic Rule we create but would need to add a custom product pricing rule for tax related Features modules that directly calls your particular component. In fact, doing this, you could just name it something entirely different.

What I'm wondering about this patch in particular is still if we can't figure a solution higher up. I don't think we should have to check for the existence of the same Rule configuration, because I'm guessing that either technically every site should only ever have one default rule of the same name or Rules may have a way to deal with these. Maybe it really should just blow away the pre-existing one, in which case we could simply adjust the module weights w/ a little hook_hook_info() I think so that the Features module's default rule configuration hook goes after the Tax module.

The reason I'm pushing back here is that we just had a similar patch get posted to avoid the array_merge_recursive() in module_invoke_all() in the event that two order states of the same name are defined on a single site. As long as we don't have any semblance of namespacing for all these systems we define, we'll have plenty of places for collisions short of module and site developers ensuring they have unique global naming conventions. Your patch alone will fix the immediate problem, but it also will cover up other issues - for example, the action to calculate taxes by type shouldn't be checking that same boolean, otherwise we could just tweak the Features export and call it good. : P

Read through the exchange again, and I'll be fine with the original approach if we can be certain that rules_config_load() instead of hook_default_rules_configuration() won't be susceptible to a race condition. What happens if the module defining the alternate default component for that tax rate is weighted higher than the Tax module? I guess it'll just replace the default component defined by the Tax module?

Ok, thought through this more today, and I think I have a justification for adding this exception for the definition of tax rate components. Part of it is that you can't just specify rules_component = FALSE given the current system as described in comment #5. But the deeper question I see is if we're making exceptions for this one particular set of default rules, why don't we do this for every default rule we define? And I think the answer lies in the naming of the components. The code assumes the naming pattern commerce_tax_rate_[name], when it should have simply stored the name of the rules component in the tax rate. Then we could've had any module defining any rules component it wanted and associating it with the "Calculate taxes: Tax type" actions. A separate boolean could've been used telling Tax to go ahead and define a default component for the rate.

How does this look? No more magic naming and an updated default_rules_component boolean on tax rate info to tell the Tax module whether or not a default component should be created for a rate. The patch also updates our usage of rules_config_delete() which somewhere along the way changed to require an ID instead of a machine-name. : ?

Great, and the final commit here also removes the automatic deletion of tax rate Rules components on tax rate save.

Commit: http://drupalcode.org/project/commerce.git/commitdiff/5af0c94

Just for the record, I've given a try to commerce features updating to the latest dev of commerce that includes this improvement.

The behavior is the following:
If you create a feature with some tax rates, they get created as expected in a new site.
If the site had already some tax rates with that naming, the feature is marked as overriden and taxes remain as they were.
If you override the feature, all taxes are overriden, which maybe is not as granular as it would be desirable, but I'd say that's features expected behavior.

I don't see that this patch has affected commerce features, but it may be worthy to give it a second look.

Rock on, happy to hear it. : D