use hook_field_widget_form_alter() instead of hook_field_attach_form() for adding tax to price field widgets [#1791226]

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

Comment	File	Size	Author
#6	1791226.6.commerce.tax-price-hook_field_widget_form_alter.patch	5.68 KB	joachim
#6
#1	1791226.commerce.tax-price-hook_field_widget_form_alter.patch	5.23 KB	joachim
#1

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

joachim CreditAttribution: joachim commented 21 September 2012 at 09:57

Status:

Active

» Needs review

File	Size
1791226.commerce.tax-price-hook_field_widget_form_alter.patch	5.23 KB

Comment #2

rszrama CreditAttribution: rszrama commented 2 October 2012 at 13:29

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?

Comment #3

joachim CreditAttribution: joachim commented 2 October 2012 at 13:39

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)

Comment #4

rszrama CreditAttribution: rszrama commented 2 October 2012 at 14:14

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?

Comment #5

joachim CreditAttribution: joachim commented 2 October 2012 at 15:16

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 :)

Comment #6

joachim CreditAttribution: joachim commented 10 October 2012 at 10:21

Status:

Needs work

» Needs review

File	Size
1791226.6.commerce.tax-price-hook_field_widget_form_alter.patch	5.68 KB

Here's a rerolled patch with that in.

Comment #7

rszrama CreditAttribution: rszrama commented 12 October 2012 at 21:58

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. : )