[Vtigercrm-developers] vtigercrm-developers Digest, Vol 13, Issue 4
Ken Lyle
kenlyle at yahoo.com
Tue Jan 9 10:02:21 PST 2007
In my wildest imagination, I imagine that some team
might decide to prune the database design, then
regenerate the entire guts of the application using a
code generator like CodeCharge or PHPRunner, or
better.
Then, if there is a quality issue, there should be
only one place to look.
Referring to Dennis Grant's rant, for example : If
there were a wish to add more error checking on query
construction, it would only have to be added in one
place. Then, just regenerate the application.
Of course, it wouldn't look like vTiger, but I am sure
that one of you wizards can come up with a way to
match all the generated code with user interface
elements in the vTiger 'skin', like tabs and lists and
stuff.
I don't think that this ridiculous. In fact, it might
be a good idea.
Ken
--- vtigercrm-developers-request at lists.vtigercrm.com
wrote:
> Send vtigercrm-developers mailing list submissions
> to
> vtigercrm-developers at lists.vtigercrm.com
>
> To subscribe or unsubscribe via the World Wide Web,
> visit
>
>
http://lists.vtigercrm.com/mailman/listinfo/vtigercrm-developers
> or, via email, send a message with subject or body
> 'help' to
> vtigercrm-developers-request at lists.vtigercrm.com
>
> You can reach the person managing the list at
> vtigercrm-developers-owner at lists.vtigercrm.com
>
> When replying, please edit your Subject line so it
> is more specific
> than "Re: Contents of vtigercrm-developers
> digest..."
>
>
> Today's Topics:
>
> 1. A rant about Error Checking, Lack Of in the
> CRM (Dennis Grant)
>
>
>
----------------------------------------------------------------------
>
> Message: 1
> Date: Tue, 9 Jan 2007 11:16:57 -0500
> From: "Dennis Grant"
> <dgrant at accuratetechnologies.com>
> Subject: [Vtigercrm-developers] A rant about Error
> Checking, Lack Of
> in the CRM
> To: <vtigercrm-developers at lists.vtigercrm.com>
> Message-ID:
>
>
<3E26E7A199CABA49822B3E6B741434F97D0933 at exch.accuratetechnologies.com>
> Content-Type: text/plain; charset="us-ascii"
>
> 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.htm
>
> ------------------------------
>
> _______________________________________________
> vtigercrm-developers mailing list
> vtigercrm-developers at lists.vtigercrm.com
>
http://lists.vtigercrm.com/mailman/listinfo/vtigercrm-developers
>
>
> End of vtigercrm-developers Digest, Vol 13, Issue 4
> ***************************************************
>
More information about the vtigercrm-developers
mailing list