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

[Dennis Grant]

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.vtigercrm.com/pipermail/vtigercrm-developers/attachments/20070109/a7204a02/attachment-0004.html