[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