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

Downside:

- 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

Files: 
CommentFileSizeAuthor
#6 1791226.6.commerce.tax-price-hook_field_widget_form_alter.patch5.68 KBjoachim
PASSED: [[SimpleTest]]: [MySQL] 3,551 pass(es).
[ View ]
#1 1791226.commerce.tax-price-hook_field_widget_form_alter.patch5.23 KBjoachim
PASSED: [[SimpleTest]]: [MySQL] 3,589 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new5.23 KB
PASSED: [[SimpleTest]]: [MySQL] 3,589 pass(es).
[ View ]

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?

It's not that clear from http://drupal.org/node/542202 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)

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?

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

admin_menu/admin_menu.info
8:dependencies[] = system (>7.10)

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

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

Here's a rerolled patch with that in.

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.

Issue summary:View changes

Updated issue summary.