CiviCRM Community Forums (archive)

We've been having lots of conversations about improving our code infrastructure & underlying technologies. But, I feel a bit like we are trying to fix up our neighbourhood by fixing the potholes in the roads & adding some new traffic lights and new pipes - and behind all this is the hope that somewhere along the way the gang clubhouse on the corner will quietly shut up shop and leave. But, we can't get rid of the gang because we don't have roads that will carry in armed forces and we can't build the roads because the gang won't let us. And so we are going about our meetings discussing the roads & ignoring the elephant in the room - a mob of hairy, hygenically-challenged bike-riding ruffians.

To my mind the hell's angels in CiviCRM code is a few particularly challenging flows. Mostly in code that has been around a while. Of course there are loads of wannabe gangs (copy and paste code) but those ones probably will be easy to move out when we gentrify the neighbourhood.

So, the biggest clubhouse, to my mind, is in the contribution form. This is probably the most complicated, hardest to read & easiest to break code in CiviCRM & unfortunately this clubhouse is on High Street!

How do we tackle it?

- build a bypass?
- catch the gang members one-by-one until we get the numbers to a manageable number
- send in the roading contractors & hope there is no carnage
- set up roadblocks around the clubhouse - let gangsters out but not back in

I feel like unless we build a complete bypass we need to chip away at the gang strength & keep it from rebuilding before we can send the contractors in. A bypass is not without appeal but we would also miss out on all the good things high street has to offer  - unless we can figure out exactly what they are & rebuild them elsewhere. The problem is those pesky gang members keep attacking the nice council workers we send in to do an audit.

So what does chipping away at the gang mean? A multifaceted approach of social workers & force to slowly reduce the gang to a manageable problem? Combined with unit tests to keep the gang members who have left from returning?

I took a look specifically at the postProcess function of the confirmation form. It seems the thing that makes it so challenging to work with is that the $form object & to a less extent various arrays are passed around from pillar to post through various functions - sometimes the same complex function more than once and these functions are setting things on the form object or relevant array - with it being extremely hard to figure out if there is any impact & if so where.

From what I can tell the reason for passing things as a reference is often more to do with earlier php versions than code requirements.

So, one way to tackle this is to start at the end & investigate whether things that are being set are being used elsewhere & otherwise only set them in variables local to the function ie

https://github.com/eileenmcnaughton/civicrm-core/commit/989aa1eee96193a034efe5b1141b141a2c278de3

- in this case we defined a local array & pass that out - with the goal of using the local array earlier & earlier in the function as we  investigate the hidden implications of them being set in the far-down-the-chain function. This is rather painstaking - but does mean incremental code improvements & with several people working towards it would make the problem more manageable. One specific problem is that the function called at the end of the confirm postProcess is also called earlier on (& forum posts suggest that there is a bug that occurs when it is called twice in some circumstances). This makes it hard to 'work backwards from the end'

https://github.com/eileenmcnaughton/civicrm-core/blob/4.4/CRM/Contribute/Form/Contribution/Confirm.php#L972

My feeling is that it would be really good to have a few people have a go at picking off a single gang member each & meet back & discuss afterwards & take some learning out of that & go forwards.

The other issue on this approach is that we need to prevent them getting back in - both by the formal road blocks (unit tests) & by intelligence officers - people testing the form. Unfortunately we need some pretty good intelligence officers to understand how we can be sure the gang member removal went well. We need to figure out how we can tell if gang members are sneaking back in or leaving destruction in their wake. We also need unit tests - but until we chip away enough at the gang members it's hard to put them in - ie. if we can change a function so it has clear inputs & outputs we can test it - but if the form object or an array reference is being passed in & things are being done to it that may or may not matter, or even be right - we need to unwind that somewhat before testing it.

Another approach is to start with functionality & refactor chunks of it with a more holistic approach. This is less soul-destroying - but possibly more dangerous that the code-crawling approach.

I think my general recommendation would be to do a small attack with preferably 4-5 people on a particular clubhouse, regrouping and planning the next steps. Looking for intelligence officers, social workers, road builders & engineers - even a few stop-go-men (or women). I think we actually need to focus on the clubhouse rather than hoping it will move on quietly to another project or even re-integrate into society.

 

Unit tests as road blocks inside the neighbourhood, and functional tests as roadblocks around the neighbourhood! I need to play some Grand Theft Auto to extend these metaphors some more! 

Sounds like some earlier limitations in PHP and standard techniques of the era when CiviContribute was first coded still have traces in the code.

Personally I've never been fond of the convenient but dangerous method of passing large arrays by ref, which is relatively common in some parts of our codebase (eg $params arrays). Drupal Forms API has evolved over the years away from having a lot of shared data in particular well documented structure being passed around to, I believe, locking the data down more as private to appropriate routines, but I haven't followed that codebase that closely.

Perhaps totten or others more familiar with these things can suggest better, more modern techniques. While PHP is not a strongly typed language, it does support type hinting http://www.php.net/manual/en/language.oop5.typehinting.php. It would require much greater use of interfaces it seems, and to make it really useful perhaps the SPL Types library which some documentation states is experimental and other states is stable. What is the Symfony way here? Is there something along the lines of Doctrine Annotations for arg types?

Personally I've never been fond of the convenient but dangerous method of passing large arrays by ref, which is relatively common in some parts of our codebase (eg $params arrays).

On the api, params is an array by value (php is clever enough to do a copy only if they are modified)

Going back to Eileen shiny initial post:

They are a few places that are for grown up, and the contrib code definitely belong to that category. I think part of the problem is that we have bolted too many things into the same mother of all forms. Part of the problem are functional too, eg does it really make sense to have the same suggested amounts if its a one shot or monthly donation?

Might be better to keep the form, but write more focussed ones (eg only single donation, with a focus on having nice looking price (images+longer texts) instead of the radio.

A bit like the searches, you got one giant advanced search, but most of the times you are using simplier ones that are tailored and easier to handle (it's a bad analogy code wise, but for UI, could work IMO)

With any other form I might say, "lets migrate it to our new form abstraction once that's built" but contribution/membership/pledge/pcp are probably the most difficult things we'll have to add to that new system and so probably will be the last pieces we do... so I'd imagine it's going to be a while.
Chipping away at bad code and adding unit tests is always in season as far as I'm concerned.
Still, I do think that the biggest bang for buck with any civi contribution right now is pushing that new form system forward.

With any other form I might say, "lets migrate it to our new form abstraction once that's built" but contribution/membership/pledge/pcp are probably the most difficult things we'll have to add to that new system and so probably will be the last pieces we do... so I'd imagine it's going to be a while.

I agree that contribution/membership/pledge/pcp are the most difficult cases, but I also think they're the most important cases where our users want to do the most customization. They're the crucible: if a new form abstraction fails to improve those, then it fails. If we focus our design discussion on handling those well, then everything else ought to be easy. I like the work that Chris Burgess started at https://gist.github.com/xurizaemon/6108f859cdc6aa39da9f -- it might make sense we if started out by getting ~10 mockups representing the ideal forms/layouts/pageflows for ~10 different orgs/use-cases (based on the opinions of ~10 different people), and then we craft our design to allow those. As you say, it's going to be a while before the implementation is there, but we can/should work on the design/vision. A more concrete design/vision will help guide our cleanups/refactorings/changes.

Actually, I kind of think we should just block it off, and provide ways to build contribute forms that aren't the current system.

Which takes me to the more API driven approach anyway, since Contribute's business do-everything is half my problem, and Quickform the other half: if I could build these forms from components then I'd do that.

Baby steps perhaps - build basic forms which do basic things using API only, and rebuild from there until we have replacements which are more modular.

At some future point, wrap up the existing Contribute forms as an extension/optional element which sites can retain if they are heavily invested in not moving away from the current model.

might be good to explore building a contribution form like this:

http://bradfrostweb.com/blog/post/designing-an-effective-donate-form/

using angular and the contribution api

lobo