CiviCRM Community Forums (archive)

This is a follows up on some of my comments about it in this blog post
http://civicrm.org/blogs/eileen/you-owe-me-3-tests-function-kitchen-sink

The anti-pattern we need to fix is to have the BAO re-implementing a function (create or delete) that kind of exists already in the DAO (or its parent object) but does it by instancing an new object of the same DAO class it inherits from (details below).

Lobo mentioned the two reasons:
a) the parent function doesn't work the way it's useful (eg. the del method expects the id attribute to be set previously but doesn't accept the id as a param
b) the del function is not static and a common usage pattern is to call BAO_entity::del(id)

The issues of that pattern
1) every BAO uses a different method name (eg. create vs add or delete vs. del vs. removeParticipant) making is super hard to guess what to use (or have a generic API function)
2) having a BAO (that inherits a DAO) AND instanciate a new DAO of the same class is OO ugly and and makes allan kay cry a little.

Solution(?)
1) the existing DAO functions aren't good enough, add in the CRM_Core_DAO the two (or whatever needed)
del (id)
create (params,ids)
And each BAO uses (or override if needed) these functions in the BAO instead of implementing a custom workaround on every BAO in a slightly different way

2) if static is a problem still, change the calling pattern
so instead of
$newParticipant = CRM_Event_BAO_Participant::create( $formatted , $ids );
it's
$bao = new CRM_Event_BAO_Participant();
$newParticipant = $bao->create( $formatted , $ids );

As a side node, the object hierarchy is quite a russian doll
BAO_xx->DAO_xx->CRM_DAO_Core->DB_DataObject->DB_DataObject_Overload

details. This was an email in july last year that with lobo & eileen where I mentioned some coherency issues with the BAO that we experienced.
_______________________________

Hi,

Was puzzled by that code in  CRM/Event/BAO/Participant.php

1) Why is this method called deleteParticipant instead of overriding delete ?
(I'm assuming no good reason)

2) why is this instantiating a new CRM_Event_DAO_Participant instead
of calling
parent::delete() ?

The real question is about the second one, the pattern of creating a
new DAO object instead of calling the parent is quite common in Civi,
and never quite understood why.

X+

To clarify: I was wrong at the beginning and indeed the "high level" methods that you have defined in the BAO are indeed having a functional perimeter that is different than the DAO one.

I agree that overriding the DataObject methods was not a good suggestion.

However, we need having higher level methods that basically match the API create & delete and are now solved independently again and again in (almost) each BAO. This is bad because they don't have the same name even so they fill the same need on different classes (look at what is done in api/v3/utils.php for the generic functions).

It means that you never know what is the right method to delete an object. Is this remove? delete? deleteParticipant? None of the above? same to create a new entity. Is this create? add? Is the id the second parameter? Is this an array or can it be an int? what's the index that need to be used?

The CRM_DAO_Core should implement (abstract if needed) the "official" methods to create and delete (and update and get, but that so far as not been a major problem, probably because get/query is a topic on his own)

So what I would like is that the CRM_DAO_Core define these two create & del and define what params they have one for all, and that each of the BAO override them instead of being either del/createParticipant/create/add/whatever

You have no idea how much it makes contributing to civi more difficult, because when you start you are not too sure what is where, and even one different case forces you to destroy the mental model you started building and you end up questioning and being unsure of everything

X+

Just a couple of thoughts:

1. Generalizations dont really help anyone a whole lot.

2. The BAO's are fairly consistent. Most BAO's do have: create, add and retrieve and take the same set of params. We diverge a bit with delete (since that word was already taken). However when u r talking of a lot of objects, being fairly consistent does not help, its 100% or nothing

3. Deciding the sets of rules (aka what Eileen wrote), having a set of generic tests to enforce the rules and fixing the codebase to match the rules is a good concrete way of addressing the issue.

We all know and are aware of these things. Lets just be more civil and get along with fixing and standardizing things. Everyone has put in a lot of work and effort into things to get to where we are now

lobo

Yes, 'standards-welcome' is a good idea

and since we are talking about standard and technical debt, here are a few things that we can can add to the list (largely culled from the comments in the blog post):

1. Standardize on the set of functions a BAO can implement and overwrite. Analyze and see the common pattern and add them as a virtual function with default behavior at the top level that classes can over-ride. Note that for most simple objects (the vast majority), we basically just use the DAO functions.

2. Implement and format all the files in {drupal,wordpress,joomla,api,CRM}/* to follow drupal coding conventions. Also implement a commit hook to verify and validate this on a svn commit and all patch file attachments.

2a. Also decide on smarty coding templates, seperation of tpl and js

3. Eliminate eval from more places in the code base

4. Move business logic from the forms to the BAO

5. Cleanup and centralize the token code.

6. Start auto-generating documentation for internal classes and prioritize the classes in most needs of doc/inline help

7. Start experimenting and migrating part of the core code towards Symfony2

I'll add more to this list. Maybe we should move this to a wiki page

lobo

1) Every BAO will have a create function. This will be called by the API & form layer
2) the create function will take a single params array
3) Depending on the params passed in the create function will perform any additional actions like creating activities
4) The create function will call hooks.
5) some form of standardisation around $ids object being passed in.....
6) The add function if it exists will be internal to the BAO layer
7) Every BAO will have a delete function
The delete action will take any additional tasks like deleting additional object
9) The delete action will take an array including ['id']
10) The api & form layer will call the delete action
the API & form layer will call the delete action

IMO, we must put have the create & retrieve & delete function in a common parent of all the BAOs. This means CRM_DAO_Core. We must so
1) we can put the common behaviour (eg. calling the hook) there
2) we can document better and once for all the params & how that works
3) we can avoid copy paste the comments between BAOs (I'm assuming it's common pattern to look for the parent method if there isn't any doc)
4) Some method can be abstract, forcing the BAO to behave and override the method like intended

1. Standardize on the set of functions a BAO can implement and overwrite. Analyze and see the common pattern and add them as a virtual function with default behavior at the top level that classes can over-ride. Note that for most simple objects (the vast majority), we basically just use the DAO functions.

the dao is expecting that you set the attributes and then call the action (without parameter) but the BAO mostly have methods that are static and that takes the data as a param (and you can't override a method with a static one). ie. I think the CRM_Core_DAO should implement the 4 CRUD "the BAO way" and wrap the DB_DataObject methods.

Ideally, the CRUD methods are close to what Symfony2 has and replacing DB_DataObject by whatever is what is in Symfony will be super easy (one can always dream

Something I did use on the api/class.api.php is to use polymorphism when it made sense.

For instance the get method can either have the usual array $params or an int (that is taken as the id as get(array(id=>42)) or as a string that is evaluated as a json representation of $params

For the BAO, I would find useful that del (or whatever its name) can either have an array of int (delete a batch) or a int (delete a single)

That would avoid a lot of boilerplate del (array ($id)) in the calling code

2a. Also decide on smarty coding templates, seperation of tpl and js

YES! I'm spending a lot of time with mustache and writing jquery plugins start having some ideas about what should be done (eg wrapping everything in an anonymous function...)

Something related that wasn't mentioned in this brainstorm sprint:

1) never inline js/no inline style
2) ok to have <style> or <script> in a template if local to this template only
3) never html generated in the PHP (actionLink...)
4) never business logic in the templates  ({if $action eq 1 or ...})
5) never say never


X+

OK - I think a WIKI page is the right place to start it

One more I wanted to add for the standards - pseudoconstants - there is some variation in input & output params that I'd like to see eliminated

And, on the clean-up - I think that a list of things that can be done with a fairly low entry point is a good idea. I'm a big fan of the report framework but it would be good to follow through on this code comment, for example. The idea of having a list of tasks appropriate to people with less experience with CiviCRM and / or with php has been mentioned before.

    // select() method below has been added recently (v3.3), and many of the report templates might
    // still be having their own select() method. We should fix them as and when encountered and move
    // towards generalizing the select() method below.
    function select( ) {
        $select = array( );

Hi, I have moved a good chunk of the ideas from the later part of this thread into that page & expanded it (if I have missed stuff it was omission not intention & please add it)

I have ALSO written an API standard! And removed the out-of date information about how to write an API (which was causing people to diligently implement things we had started to phase out  )

Am not sure what all the items on the list of standards apply to - I think fleshing that out is first priority