Function extraction (Move towards generic custom data support for all entities)

[eileenmcnaughton]

Overview

Basic code clean up

(reviewer's commit from 28bbc8d)

Before

Code not re-usable

After

Code re-usable

Technical Details

@mattwire this is a review commit of your patch here
28bbc8d

In general I think your patch makes sense as an extraction & I thought it worth splitting this part off & getting it merged so we could build on it. I still have thoughts about whether this is the best place for the extracted function (back to our traits experiments) but I think a refactoring can have several steps & this is a step in the right direction.

Changes I made from yours

1. I left off the change in utils.php as I didn't know the reason
2. comments changes
3. This is the significant one - I changed the order of the params in addToForm as
   a) they didn't seem to match what was being passed in from the membership form and
   b) it seemed that subName would be more often used

Comments

@mattwire if you confirm that you think this sub-commit is good to go I will merge as a reviewers's commit

[eileenmcnaughton]

@mattwire to tease out a bit more of the bigger approach....

I have some thoughts -

1. we looked at using the trait approach - (ie. this was my first cut & you did a second one ). What I potentially LIKE about the trait approach is we could include it from the parent by simply using an 'if function exists' type function. I don't think we can 100% prevent having to 'mark' each form that we want custom data added to in some way - a property, a function, a trait inclusion or whatever - since we need to do some form of whitelisting

2. I really want to get my head around syncing the address custom data block into our overall approach - it adds some extra challenges - but they also apply to email, phone etc blocks

3. once we have replaced those 3 repetitive lines with addCustomDataToForm I think it makes sense to re-cut the functions that make up addCustomDataToForm - there is not a great logical separation between what they do IMHO and they are hard to add a test to. I'd rather see the extraction of params from the form & urls distinct from the handling of them

4. I'm still not convinced we should enable any additional entities by default - only that they should be enable-able. (FWIW the 2 entities I currently care about are RelationshipType and Email)

5)if we are doing a lot of form-intensive testing we might want to take the chance to convert some datepicker fields on those same forms while we are at it.

[mattwire]

• (r-explain) PASS
• (r-test) Undecided
• (r-code) PASS
• (r-doc) PASS (functions have detailed docblocks)
• (r-maint) PASS (stepping stone towards common methods to add custom data to forms)
• (r-run) Undecided: PASS (works as before)
• (r-user) Undecided: PASS (no change for user)
• (r-tech) Undecided: PASS

This is a code improvement that will make it easier to add custom data to any form.

[mattwire]

@eileenmcnaughton Merge on pass

[monishdeb]

Merging as per tag.