[Vtigercrm-developers] A rant about Error Checking, Lack Of in the CRM
richie
richie at vtiger.com
Fri Jan 12 01:17:29 PST 2007
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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.vtigercrm.com/pipermail/vtigercrm-developers/attachments/20070112/57698999/attachment-0004.html
More information about the vtigercrm-developers
mailing list