[Vtigercrm-developers] A rant about Error Checking, Lack Of in the CRM

Joe Bordes joe at tsolucio.com
Fri Jan 12 02:37:38 PST 2007


Hi Richie,

I would really like to help. But my experience has been that you don't
want that help.

I have asked many "developer" questions on this list and have received
no answer. I am adding modules and code to vtiger and need to know the
changes so these can keep up
I have directly asked questions about vforge to no avail
I have sent email thru web asking to colaborate and host vtiger.es ->
nothing
I have offered my help with exact details -> nothing
I was actively helping in the forums and wiki -> doesn't to seem
important

Sorry to be so harsh, it is not my intention (email is very cold). I am
just stating my personal experience and feelings. If you can get in
contact with me, I am very interested in establishing a relationship
with VTiger and helping out.

Regards, Joe
TSolucio

El vie, 12-01-2007 a las 14:47 +0530, richie escribió:
> Dennis you are bang on target.
> Yes, we do have inconsistencies of the kind you have mentioned and
> some worse too ;-).
> 
> Thanks for pointing these out.
> If you have made some fixes to these kinds of issues, do contribute
> the same back and we will have it integrated into the core.
> 
> The code can be vastly improved team and we need your help. This is
> something that I have always asked for. We are not geniuses but we are
> willing to learn. 
> Yes there are pretty, pretty bad practices in the code but then not
> all the code is junk. Together we can clean it up much much faster.
> 
> Richie
> 
> Dennis Grant wrote: 
> > OK gang, time for me to go off on a rant here:
> > 
> >  
> > 
> > I recently had a ticket where a user – a manager – was complaining
> > about how the HelpDesk-TroubleTickets module had two views called
> > “All”, and they had different definitions.
> > 
> >  
> > 
> > I had a look inside the vtiger_customview table, and sure enough,
> > there were two rows in HelpDesk titled “All”; one with a viewed of
> > “64” and one with a viewed of ‘95”.
> > 
> >  
> > 
> > I was able to determine that viewed = 64 was the correct one. It
> > looked to me like somebody had created a custom view titled
> > “All” (that ought to be a reserved word and should be rejected as a
> > possible custom view name) and the user interface (soundly) doesn’t
> > allow a view named “All” to be deleted.
> > 
> >  
> > 
> > So I deleted that row from the database – and all hell broke loose.
> > All of a sudden, I couldn’t view the ListView for the HelpDesk
> > module any more; instead, I got the familiar “PearDatabase” error
> > which means some Sql statement somewhere is screwed up.
> > 
> >  
> > 
> > So I go “whoops!” and I put the line back in the table – and that
> > DOESN’T FIX THE PROBLEM.
> > 
> >  
> > 
> > Oh. Shit.
> > 
> >  
> > 
> > It takes me a few seconds to work out that something, somewhere has
> > associated my user with the cvid of 95 and my yanking it out has
> > broken that association. I go looking for where that association is
> > stored (FOR USER admin MODULE helpdesk USE LISTVIEW CUSTOMVIEW 95)
> > but I can’t find it.
> > 
> >  
> > 
> > So I go into the code itself, and discover the culprit is the line:
> > 
> >  
> > 
> > $list_query = $oCustomView->getModifiedCvListQuery($viewid,
> > $listquery,"HelpDesk");
> > 
> >  
> > 
> > In HelpDesk/ListView.php. $list_query is coming back as “select ,
> > foo bar blech”, and then later on that query is executed. This query
> > not being proper SQL syntax, it bombs.
> > 
> >  
> > 
> > As a quick fix, I add the following hack:
> > 
> >  
> > 
> > If ($viewid == 95) {
> > 
> >    $viewid = 64;
> > 
> > }
> > 
> >  
> > 
> > Which restores functionality – whew! – and then I go looking to see
> > what the hell happened.
> > 
> >  
> > 
> > I am not a DBA; my database experience up to this point was a couple
> > of basic SQL and database classes in college, and some basic
> > database reporting apps in the 20-odd years since. I’m learning the
> > DBA stuff on the fly. I was not aware of “ON DELETE CASCADE” foreign
> > key constraints, and the concept that deleting a row from one table
> > might cascade through other tables. I am now. I discovered that
> > there are a number of tables set up that use the vtiger_customview
> > table as a foreign key, and have the ON DELETE CASCADE constraint on
> > them. Thus, deleting the row from vtiger_customview deleted rows
> > from vtiger_cvcolumnlist (and a few others) which explains why my
> > putting that row back in didn’t fix the problem. I guess I need to
> > learn about rollbacks.
> > 
> >  
> > 
> > That’s *MY* mistake. It is always dangerous to muck around with the
> > brains of an application without knowing all the ramifications
> > beforehand. Something innocent looking turned out to have deeper
> > implications than I assumed, and I got myself into a spot of
> > trouble. My Bad.
> > 
> >  
> > 
> > But – and this is what I want to rant about – there is blame to
> > share here. Because in further debugging the chain of events that
> > led to a PHP error being displayed to a user, one sees that the CRM
> > code is making all kinds of assumptions about its data. The
> > application is positively RIFE with:
> > 
> >  
> > 
> > $sql_query = buildQueryDynamically();
> > 
> > $result_set = sql->execute($sql_query);
> > 
> > doSomething($result_set);
> > 
> >  
> > 
> > Where NONE of these procedures EVER checks for errors.
> > 
> >  
> > 
> > Here’s an actual example:
> > 
> >  
> > 
> > function getCvColumnListSQL($cvid)
> > 
> >         {
> > 
> >                 $columnslist = $this->getColumnsListByCvid($cvid);
> > 
> >                 if(isset($columnslist))
> > 
> >                 {
> > 
> >                         foreach($columnslist as $columnname=>$value)
> > 
> >                         {
> > 
> >                                 $tablefield = "";
> > 
> >                                 if($value != "")
> > 
> >                                 {
> > 
> >                                         $list = explode(":",$value);
> > 
> >  
> > 
> >                                         //Added For getting status
> > for Activities -Jaguar
> > 
> >                                         $sqllist_column =
> > $list[0].".".$list[1];
> > 
> >                                         if($this->customviewmodule
> > == "Calendar")
> > 
> >                                         {
> > 
> >                                                 if($list[1] ==
> > "status")
> > 
> >                                                 {
> > 
> > 
> >                                 $sqllist_column = "case when
> > (vtiger_activity.status not like '') then vtiger_activity.status
> > else vtiger_activity.eventstatus end as activitystatus";
> > 
> >                                                 }
> > 
> >                                         }
> > 
> >  
> > 
> >                                         //Added for for assigned to
> > sorting
> > 
> >                                         if($list[1] == "smownerid")
> > 
> >                                         {
> > 
> >                                                 $sqllist_column =
> > "case when (vtiger_users.user_name not like '') then
> > vtiger_users.user_name else vtiger_groups.groupname end as
> > user_name";
> > 
> >                                         }
> > 
> >  
> > 
> >                                         $sqllist[] =
> > $sqllist_column;
> > 
> >                                         //Ends
> > 
> >  
> > 
> >                                         $tablefield[$list[0]] =
> > $list[1];
> > 
> >                                         $fieldlabel =
> > trim(str_replace($this->escapemodule," ",$list[3]));
> > 
> > 
> >           $this->list_fields[$fieldlabel] = $tablefield;
> > 
> > 
> > $this->list_fields_name[$fieldlabel] = $list[2];
> > 
> >                                 }
> > 
> >                         }
> > 
> >                         $returnsql = implode(",",$sqllist);
> > 
> >                 }
> > 
> >                 return $returnsql;
> > 
> >  
> > 
> >         }
> > 
> >  
> > 
> > This function assumes everything works – there’s no test to see if
> > the $cvid parameter passed in is not null. It checks to see if
> > $columnlist isset before doing stuff, but has no code in the case of
> > $columnlist NOT being set. There’s no test to check for $columnlist
> > being set, but containing no rows. It COULD attempt to fill in
> > reasonable defaults. It COULD flag the error back higher to the
> > function that called it. There’s a million different things that it
> > COULD do – but instead, it fails silently, returning bogus data,
> > which breaks the upstream code.
> > 
> >  
> > 
> > This function is only half done. It works for cases where everything
> > works as expected, but fails spectacularly in cases where the
> > unexpected happens:
> > 
> >  
> > 
> > - what should it do if $columnslist fails the isset test?
> > 
> > - what should it do if $columnname or $value is NULL?
> > 
> > - what should it do if the explode() fails (meaning that the data in
> > value is not in the expected format)?
> > 
> >  
> > 
> > And this sort of thing happens ALL OVER THE PLACE in the CRM code.
> > There is a persistent and pathological lack of error checking and
> > handling.
> > 
> >  
> > 
> > This needs to be rectified.
> > 
> >  
> > 
> > Please, please, PLEASE – if you are writing new code for the CRM,
> > take the time to do the error checking and handling!
> > 
> >  
> > 
> > DG
> > 
> > 
> > 
> > ____________________________________________________________________
> > 
> > _______________________________________________
> > Reach hundreds of potential candidates - http://jobs.vtiger.com 
> 
> _______________________________________________
> Reach hundreds of potential candidates - http://jobs.vtiger.com 




More information about the vtigercrm-developers mailing list