Do you like to whinge about CiviCRM code? Have you sat through others doing having a rant? I've certainly done both. Being in the drupal world people often like to compare CiviCRM code with drupal & CiviCRM usually comes up a bit short. I think that's like comparing my kitchen with Bill Gate's kitchen. There are a few good reasons why my kitchen is not as nice as his. However, should I look at his kitchen (in a magazine) then I might glean a few good ideas that I could use in designing my own. (Copying the colour scheme would be in my budget :-))
To continue the metaphor, Bill Gates gets to rip his kitchen out & put a new one in whenever it's getting a bit tatty whereas I have to patch mine up & incrementally improve it. Unfortunately, each time I get a new whiz-bang gadget I find it harder & harder to get the cupboard door to shut. And then I can't find anything or open the door without everything falling out.
This is called kitchen debt. I have a lot of kitchen debt due to my inability to manage the expectations of those for whom I manage the kitchen. Unfortunately they tend towards unrealistic timeframes, constant demands, quick (sticky, unhealthy) fixes over durable ones and fights over whose turn it is to use the batman fork. Every now & then I have to have a massive tidy up, throw some stuff out, consolidate other stuff & rationalise whether I could replace 8 saucepans with one really good one. I also take a wee look at at the kitchens I can't afford to see if I can think of some affordable variations on them.
The technical term when you code looks like my kitchen is Technical Debt. Like kitchen debt, we all have some, but depending on our resources, our clients demands, whether we share the kitchen & our own natural tidiness we have more or less of it. In some areas CiviCRM is doing really well at redressing technical debt. The documentation team have done an amazing job. However on the wikipedia page above this sounds very familiar.
Delayed Refactoring. As the requirements for a project evolve, it may become clear that parts of the code have gotten unwieldy and must be refactored in order to support future requirements. The longer that refactoring is delayed, and the more code is written to use the current form, the more debt that piles up that must be paid at the time the refactoring is finally done.
There are a bunch of complaints I could share about aspects of the CiviCRM code but my goal in writing this is to get you to do that. I want to put technical debt on the agenda, not just in this blog but in EVERY CiviCon & EVERY Code Sprint. These days documentation, tests and the api have become part of our CiviCRM conversation and I would like to see the ongoing management of our technical debt right up there as well. We should dedicate some time at each sprint for refactoring code for the sake of refactoring, not just to fit in another gadget. Talking about what you think the biggest problems are is a really good start.
Comments
On the API side, the biggest issues are the lack of coherency of the BAO/DAO layer. Ie each BAO seems to have a different method for add/create/modify/update and that's a hit and miss.
Biggest projects are moving away from quickForm & PEAR, but not sure if it makes sense to try to fix the existing vs. implementing new features on a more modern framework
I think its a lot easier moving to an existing framework if you have tidy code & you can see what it is doing.
Almost forgot that one: on modification/creation/remove
As the BAO was only used by the form, some of the conversions & business logic was implemented in the form and not in the BAO. This works fine when you only have the form doing part of the logic then calling the BAO doing the rest, but by introducing the API, we use directly the BAO and don't use the form, so we miss some code.
We try to refactoring by moving the code from the form to the BAO, but sometimes we are guilty of duplicating the code from the form to the API (bad bad bad).
Most of the new code written is better at puting the right bit in the right place, but some of the older code would need some refactoring.
I have only minimal contact with civicrm code, as I spend most of my time in Drupal. Having said that, I'll often have to touch the civicrm api, or modifiy civicrm forms and occasionally supply the odd bug fix.
Some things I'd like to see fixed, off the top of my head:
That'll do for now. :)
http://dgtlmoon.com/wary_of_these_things_in_civicrm
Just stuff I've run into with 3.2.x branch, *hopefully* this is improved, and I don't see why not, as civicrm is a rapidly improving project.
This is not a list of CiviCRM Criticism's, but just some things to keep in mind.
Probably my biggest hate, is that last time I looked into the civicode, it was still executing it's entire codebase hook via eval(), which illustrates 2 things to me
- The codebase has a very java-esque smell to it, every operation is compounded by another object, which makes building efficient queries very difficult, most of the issues I see on the ground with problems of my clients are database performance issues caused by civicrm doing crazy long SQL joins, which are a representation of this java-esque approach of continuously extending an object and inheriting even more complex SQL patterns to your final call, instead of using PHP how it was designed for and breaking up your query into sections
Hi,
Happily some of your earlier points have been fixed (& some are more bugs / limitations that what I would term technical debt). There are others I'm unsure on the status of.
But I think your last 3 points clearly fall into the definition of technical debt. Code Comments is probably the lowest entry point into this.
NB - when I say last 3 comments - I am ignoring the rant bit & just looking @ the bullet points. I imagine Lobo pointed out that the community has to step up to change things rather than rely on him to which is something we should plan to do,
Hi,
Have you used the latest version of civi? Quite a few of your comments are about things that have been fixed.
Agree, it shouldn't be too complicated to remove the evals and would improve a lot debugging/code optimisation/cache. I did try and did manage to fix a few, but as Eileen said, we need more help. Would be more than happy to assign this one to you ;)
http://issues.civicrm.org/jira/browse/CRM-7964
As for using eval, can't say I like it either but civi is by far not alone using it and it was pretty much the only option for years in PHP.
We try very hard to avoid ad hominem comment and in general, finding *an idea* idiotic shouldn't be read as finding someone *idiot* (and wasn't there, so no idea was was said nor the topic, just general point that no matter the opinion about an idea, it isn't an opinion about the person having it)
I like the broken window link.
There has definitely been discussion about replacing QuickForm & the DB layer - these are probably held up over resources to do them more than any will to do them. I think frustration over these dominates most discussion about improving CiviCRM code. But even when we can't put in a new shelving unit it's still worth tidying up the shelves. Otherwise it just makes the job bigger when we finally do get a chance to replace the unit.
To me paying off technical debt is not about the big changes (although I'd like to see the back of Quick Form as much as the next person) but about specifically sitting down and looking for
- functions that are too long and hard to understand - look for them & split them up or rationalise them. Maybe throw a few out. Ideally I'd love to see each code sprint take a small section of code and have a big conversation about how it could be improved. I think the learning & ideas of throwing lots of minds at a small problem would flow onto a more cohesive approach on other parts of the code.
- code that is duplicated. A good example of this is in CiviReports where a lot of the functions were copied into each report. Since then someone has put some of these into the parent class so a simple but worthwhile activity would be to audit them for where code can be removed from individual reports.
-looking for functions that are in the wrong place. (the BaseIPN class holds functions that are called from the backoffice contributions that don't relate to Instant Payment Notifications in anyway)
- auditing & adding code comments
Cheers, Eileen. CiviCRM, generally speaking, I feel Civi needs to slow down and make stablility of code a higher priority. Tangentially, creating a test environment that is more realistic will help improve quality. I've even offered sponsorship. The response has been underwhelming.
What do you mean by a more realistic test environment? I've been spending a bunch developing webtests for code to go into core lately.
I have a few thoughts on that - not so much the test environment but the monitoring of how the tests are passing so that if all the tests are passing when we release 4.1.2 & then a change happens & one breaks that's visible to a list of people immeditately - not some poor sole having to figure out who did what when a month later.
It's awesome that people are making new tests. How can these tests be used to find bugs in features that are not used in the test environment?
If the environment upon which those tests are run doesn't use certain CiviCRM features, the tests cannot find a problem. The test bed for reproducing bugs is the generally the demo site. "Can it be reproduced on the demo site?" I have been asked more than once. Depending on what kind of test you are running, you may be running it on the demo site or a local install, but it's likely with the demo site data.
The demo site data has:
This list could be expanded.
I've been informed that the testing data is generated by a script, which I then looked at, couldn't figure out, and gave up on. I've offered sponsorship to enhance test data, about a month ago, with no response from Lobo yet.
Great post Eilieen. Many thanks for it. I recall some while back making a similar comment in response to a question raised about what people wanted to see in terms of a focus for future development. I suggested then that as someone who spends a good deal of time advocating CiviCRM to clients and potential clients I'd like to see a focus on getting what's already in place working as well as it can without bugs, with new features being seen as almost a second priority.
Of course with an open source project such as CiviCRM, the work is done on the areas that the contributors want to work on, and that's fine, but at the same time, at a strategic level, addressing the technical debt issues is absolutely critical in terms of moving the project to the next level.
I've been working with CiviCRM for a good few years now, and gradually - as time allows - I'm getting to learn more about how to do stuff that goes beyond the UI. Customising things a little bit, and I aspire to being able to do more in this vein in the future. If people like me are to be able to get the best out of CiviCRM it is imperative that the codebase facilitates that process wherever possible, and that is achieved in precisely the ways you and others have mentioned already in this post.
Three years ago (or even two) I'm not sure I would have agreed with Upperholme, but CiviCRM is so feature-rich today that it's hard not to agree. Sure, CiviCRM can't do everything a non-profit can imagine, but one way or another it probably can do 90% of it. It's time to take a breath and make sure CiviCRM's reliable, streamlined, and as glitch free as reasonably possible for the people who are using it. If we take care of them, those users will become advocates and donors and drive expansion of the CiviCRM user base even further.
Great post, Eilieen, and great thread here.
I like the idea of getting community resources - money and time - together to address this technical debt. If not, then the project risks having a challenger open source project develop in its space that has an infrastructure that makes development quicker, more easily testable, and more flexible to configure and modify.
QuickForm and DB layer should be replaced as a priority. I like the idea of moving to a framework like Symfony that will also be used by other projects like Drupal.
I like the idea of developing a Coding Standard for CiviCRM, and don't really care what the is in it. I think the easiest thing to do is for some of us to agree on one, then start submitting our patches after putting them through an auto-formatter like an IDE provides. My sense is that Lobo isn't so much opposed to others using them as that he doesn't personally see the value.
I found the use of eval to be an undesirable and very problematic programming pattern as well. My current IDE (Zend Studio) manages to handle it in its current version - PHP dev tools are maturing. So it is now feasible to use basic techniques like breakpoints.
I don't have a clear sense of how to do this, but I like the modularity of Drupal modules ;) - they allow maintenance to be pushed out to a responsible person or team; they reduce the daunting dependencies between various functions; and make it more possible to make changes in one area without breaking things elsewhere. I even suspect that the social infrastructure in Drupal around contrib modules has supported the development of clean separation between different parts of the code and good API's. Having a smaller 'core' might allow the core team to be a bit more agile in addressing technical debt, though at the risk of having some delay in getting features or components ready for new releases. Tough trade-offs.
Although I'm strongly in favour of tiered architecture, I don't think that it has always been well maintained in practice. Some BAO concerns itself with presentation, some Smarty templates which should be oriented toward themers are more challenging code to follow than PHP, etc. are areas that could be addressed to reduce our technical debt.
Our jQuery / javascript isn't always nicely separated into different files - I think snippets show up in Smarty templates.
Our inline code documentation is pretty minimal. That's a big barrier to entry when the programming model often means that relevant code is not in the same module. I don't really think we as a project could keep detailed inline comments up to date. I'd prefer to focus on the documentation of function interfaces and parameters, and using an automated tool to help us easily see where functions are called rather than having to rely on our IDE Search. Stepping through execution is sadly, after IRC, often the fastest (though painfully slow) way to find which functions are doing stuff.
Though a convenient standard, use of catchall variables like $params to pass stuff into and out of functions can obscure the lack of a formally defined interface for functions, and makes it difficult to debug. I would focus code documentation efforts on listing the possible contents of all parameters to functions - what are the parameters in that $params object, what are their types, and do they have any 'magic' values? I like the idea of using standard functions like the api does to sanity check parameters passed in to functions. I've also been on projects in other languages than PHP that put some of this into TEST blocks that were not compiled into releases but used in the testing phase only. I'm sure there is a good way to do that in PHP as well.
If there was good documentation for constants and function interfaces that is autogenerated like api.drupal.org then we would be much better off as a community.
There are lots of other good ideas here - like the issue of notices - but enough of my random thoughts for now.
Thanks!
Hi,
Don't like magic either, and on the params front, getfields is getting better at showing what are the "regular" fields that you can use.
We have a technical solution to explain which ones are mandatory or what are the default values (the _spec functions), but we need help to implement it everywhere.
The other filter/options params are getting better as working everywhere, we aren't 100% there either, and the self documentation isn't fully satisfying either IMO, and we would need more help on that front as well.
Last but not the least, part of the "magic" comes from the API trying to be a fairly dumb layer on the top of the BAO, and making the API would imply changing the BAO (eg moving some features from the form to the BAO or upgrading the BAO to more standards).
That's something we are slowly implementing, but takes time and even if we could get about 500 tests checking the conformance of the API to the standards and avoid magic, we haven't been able to cover every points.
X+
On that note, the new api for the cron were migrated without awareness of what we were doing with getfields in the api & their parameters are not exposed to documentation mechanisms. It's a good example of a broken window - if we don't get them sorted someone will copy them into new api without fixing.
Another broken window ;)
On the Job API side, I'm not sure the _spec were implemented and stable when it was coded, almost a good reason not to have used it ;)
This being said, we need to present this (and the basic functions and options/filter...) and write down the API coding convention & best practice.
Given that something simple (don't call the civicrm_api3 functions, always use civicrm_api) that we have writen dozen and dozen of times is still an issue today, not sure how to do it. Repetition doesn't seem to be enough
Yes, I think this along with other changes that affected the API crossed over - because the api was moving so quickly.
But if we tidy that up then it will be unclear how coding should be done in the future.
Regarding api functions being called directly rather than via the wrapper - our position for sme time has been that functions in the api must only be called via the civicrm_api wrapper - if that's not happening with the job functions then that's quite serious.
Sorry, I wasn't clear enough.
I love the work that you and others on the API team have done. My concern is with the functions in the core code in BAO/ and other directories. I want to use your approach to checking what is passed in throughout the code base, at least if we could do that through some instrumentation or non-shipped code that would affect performance. You're doing a good job and continuing to make the params and defaults more standardized and documented.
Many functions under CRM/ receive a generalized argument like that without documentation or with out-dated documentation of what the index keys could be for the $params, make some changes to the variable, pass it down to other functions that do the same thing by reference, etc. This creates tight coupling of the code, and associated problems of difficulty in debugging, maintaining, and modifying it.
As someone newly coming into the platform, from a Drupal background, there a couple things that struck me.
1. code whitespace inconsistancy. This makes it really difficult for me to generate clean patches because my editor automatically fixes those issues, so I've always got more whitespace changes than code changes in a patch. Cleaning up that would be really helpful. Add an svn commit hook that rejects code that violates that would be even better.
2. lack of automated testing. In this day and age I'd expect something as large as civicrm to have a public jenkins, cruise control, or the like. It would really be good to have a dashboard of where every commit stands with tests, and what the coverage looks like. That would be incentive to address that.
3. How drupal 6 gets packaged. I was very confused that drupal 6 wasn't in trunk, but was being planted in from a branch before release. I'd really like to see trunk able to work on all the supported platforms, because otherwise I'm very concerned that 6 support is going to break. This is something I'd be very willing to help with (making a drupal module that will work on both 6 / 7 out of trunk.
4. I'm still struggling with how to make a core change with the geo_code_level feature. There is a lot of indirection magic with the forms, and I've yet to find a good guide to that. That indirection itself is something that I'd consider debt unless it is well documented.
In addition to all of this, I'd really love it if the community was using git instead of svn. In past projects we saw contribution rates double after the conversion, because working with svn, but not being a core committer, is really difficult. Slinging git patches is so much more straight forward. I'd even be up for helping with such a conversion if folks were interested.
All that being said, I'm happy with the community, and I expect to be sticking around for a while. This is the horse I'm on. I'd just really love to see the community transformed in a way where the entry point for new developers was much simpler, as I think it would enhance the overall mission.
The push to git is in process and is being lead by Jamie. As part of the post CiviCon code sprint, I'll be working on configuring Jenkins continuous integration with the web functional tests to get all tests to execute sub 30 minutes instead of the 3 to 4 hours it takes to run the full suite of tests.
I think as refactoring work becomes more of a priority having a good testing framework that can quickly home in on regression bugs will be key. We have a cluster consisting of 7 virtual machines provisioned on the OSUOSL supercell cluster (thanks to Facebook!) that we'll be using with the new Selenium Server to increase test throughput.
If you want to help out make this happen during the sprints, come speak to me at CiviCon or anytime before that.
Young-Jin
Hi Young-Jin,
Thanks to lead that, narrowing the gap between a code change and a test fail will be really super useful.
Is this possible with Jenkins to send an email/alert when a new test fails to the one(s) having committing since the last run? and a report on IRC for general monitoring/public shaming.
Hi Xavier,
Yes, we'll have the results of the Jenkins runs fire an email to a google group for distribution and we can hook it up to the IRC channel as well since that's where a lot of the developers hang out most of the time it makes perfect sense to flash a successful or failed test run there, I've create a a kanban for the CiviCRM CI testing framework:
https://www.kanbanpad.com/projects/53cf335d2b787ccc80b2
Young-Jin
Thanx to eileen for igniting this years debate on the state of civicrm. Here are my 2 cents and responses to some of the various comments. I'll attribute it to specific authors etc.
Overall, I do think and believe that having a healthy constructive debate on what things are broken / super broken / need to be cleaned is important to the ecosystem and the health of the project. Cleaning up the code base / re-factoring various aspects of CiviCRM on a continuous basis is very important. I do think we do a fair amount of this on a regular basis, but we can and should do more of this and on a larger scale. CiviCRM is currently quite huge and many of the changes will need to be incremental. On the other hand we will need to push through some large changes throughout the codebase in one big push (the form and db layer come to mind here).
@xavier: Being more specific with what BAO's are not coherent and patching them would help. DAO's are automatically generated from the same template and are all exactly the same :). We are also exploring Symfony2 as a potential framework to move away from QuickForm. The DB layer is a bit of a unknown at this stage.
@torrance: I'm not a strong proponent of coding standards, but do admit that at this stage we probably should just adopt the drupal coding standard and modify/augment the coder modules to ensure that we can verify it. We should also validate this via a commit hook. However, I thinks we should leave third party packages as is. Any volunteers to take this on?
@torrance: 4.2 is mainly NOTICE compliant and if not there is a strong attempt to fix parts that are broken. The Views integration is currently on the lookout for a maintainer and as such has been a bit neglected
@dgtlmoon: Some of your issues have been resolved: eval has been eliminated in quite a few places (its still there in places and should be removed also). If memory serves me right, it was primarily the only way we could do it in PHP4. Apologies for any comments I might have made to you in this regard. We've taken quite a few steps forward in making it easier to deploy civicrm across test/dev/prod. This is still a work in progress. We've optimized a lot of the MySQL queries, there are still quite a few queries that we can still optimize. We probably will/should restructure search to be a set of smaller queries rather than a few humongous ones. We and other members of the community do profile the code periodically and try to fix the big bottlenecks.
@stoob: We did make a set of mistake in the 3.4 / 4.0 releases. We do feel that 4.1 stability is a lot higher than prior releases. In general people pay / sponsor features and improvements to the code base, not a lot of folks are willing to pay for a generic "improve stability of the platform". That said I do think that as we expand the suite of unit and web tests to cover more and more cases, we improve stability in a big way. You can reproduce most bugs at very small scale including ACL bugs :) There is a set of scalability and memory issues that require large data sets. I think thats the very premise of "unit testing". Create small discrete scenarios that you can test the functionality of various things.
@stoob: Regarding auto-generating a more realistic data set, we'd also like to see that happen. There is a limit of how much we can take on our plates and canvass funds for. I don't think its a trivial 10 hour project, its more like a 75-150 hours project to get it right and ensure it works on the development branch also.
@JoeMurray: Would help if you can be more specific on which BAO's concern themselves with presentation, which smarty templates are way convoluted and can be simplified. Would be great to see if the new priciest and accounting code projects that you are heading could set the standard for inline documentation within CiviCRM. That will serve as a great incentive / example to other developers in the community.
Just following on from Lobo's comments - there's a lot of good ideas come through. In some cases people are short-handing things they're raised previously. I'd like to try to drill down from some of the broader things raised to get some very specific examples of them & see if we can start moving the conversation to an action plan to address them
http://forum.civicrm.org/index.php/topic,23974.0.html
Thanks for the inspiring post, Eileen! Let's fix up some windows this code sprint! Here's some ideas for things we could redress: