This has a number of advantages:

- easier to test which fields to act on
- easier to get hold of the right form element
- code is easier to read because instead of $form[$key][$form[$key]['#language']][$delta] it's just $element
- code is easier to read because of fewer loops


- function is called once for each price field on the form
- options list of tax types is re-built each time
- requires Drupal core 7.8 or later

I don't think the first two of those are a big problem as performance in an edit form is not a huge concern; furthermore I'm testing this with 3 price fields and I don't see an impact at all

#6 KBjoachim
PASSED: [[SimpleTest]]: [MySQL] 3,551 pass(es). View
#1 KBjoachim
PASSED: [[SimpleTest]]: [MySQL] 3,589 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more


joachim’s picture

Status: Active » Needs review
5.23 KB
PASSED: [[SimpleTest]]: [MySQL] 3,589 pass(es). View
rszrama’s picture

Status: Needs review » Needs work

Hmm, do you have any ideas on the best way to document the Drupal 7.8 requirement? I'm ok with it, since we've had multiple security releases since then, but .info files can only specify major versions. Should we just add a hook_requirements() to commerce_tax.install?

joachim’s picture

It's not that clear from how to do this with core modules, but it does seem to work.

I just chucked this into one of my custom module:

dependencies[] = system (>7.16)

and the module admin page says:

Requires: System (>7.16) (incompatible with version 7.15)

rszrama’s picture

Hmm, I suppose that would fail though if Drupal were checked out from Git? Would even Git Deploy solve this in that situation?

If this hook was only added in Drupal 7.8, maybe there's a way we can check for it directly? Perhaps some other core implementation of the hook?

joachim’s picture

Not sure what you'd do about git, but admin_menu uses this already:

8:dependencies[] = system (>7.10)

... so we'd be in good company :)

joachim’s picture

Status: Needs work » Needs review
5.68 KB
PASSED: [[SimpleTest]]: [MySQL] 3,551 pass(es). View

Here's a rerolled patch with that in.

rszrama’s picture

Status: Needs review » Fixed

Ok, so bojanz reminded me that we've actually had a dependency on Drupal 7.4 since Commerce 1.2 because of some uninstallation changes, and at the time we released 1.2 I was comfortable enough leaving out the Admin Menu style notification because there had been security releases since Drupal 7.4 anyways. I'm just going to follow that model here on the off chance that making the Drupal 7.8 dependency enforced via the .info file screws up someone deploying from Git. Probably few people, but even fewer people should be on Drupal 7.8 since it was released in August 2011 and two security updates have been released since then.

I also added a drupal_static() implementation to cache the list of inclusive types. Then I realized that this was questionably more performant, because we're trading array iteration time for what amounts to duplicate information stored in memory. The tax type array is already statically cached, so we're not in danger of additional queries or anything... so I took out the static implementation I added in. ; )

Committed, all creds to joachim. : )

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.