[Vtigercrm-developers] A rant about Error Checking, Lack Of in the CRM
clement chazarra
chazarra.clement at gmail.com
Fri Jan 12 07:17:09 PST 2007
Hi guys,
I am not a really skilled developer, but I would like as well to see vTiger
evolve to a product even more stable.
As Dennis Grant, Richie and many others are outlining, there are many issues
in the source code design, security, tests, etc.
My point of view is if we try to fixes those issues without a structured
process, it will never end..
I would suggest:
1. Categories the needs of softwares in general on a Wiki:
Security / List of the possible hacks for PHP Mysql
Apache etc.
Code efficiency / No duplicated code, no dead code, etc.
Code design / Variables on top, functions written clearly and
in logic order, functions variables commented
User interface / Display, form verification, Ajax
Add other categories or modify those above, I just give an overview
of what I think
2. Once we have categories, define the rules on a Wiki:
Security / Fix SQL injections: Use the function xxx in
every sql queries
Fix ...
Code efficiency / Delete all the code commented
Delete all the code not used
Define main design patterns precisely
Recode each functions which can be
improved
Code design / Comment each variables
/ Comment each function
/ Create complete vTiger API
User interface / Manage and fix design problems
/ Manage and fix Ajax
/ Manage and fix Javascript verifications on
forms
Add other categories or modify those above, I just give an overview
of what I think
3. Wait for the release of vTigercrm-5.0.3
4. Create a Branche in vTiger Trac system named "5.0.3 Core Code
Improvement"
5. Work on files 1 by 1 according to the categories and rules
6. When file has been treated in this process once, a second check must be
performed
Those steps are not very well described, we needs more technical people to
define precisely categories and rules, but if a structure like that is
established, even if the work is huge we could at the end get a proper code
really clean... after what the database should be worked out with the same
process, and finally update the code according to the database changes.
What do you think?
Regards,
Clem
On 1/12/07, Joe Bordes <joe at tsolucio.com> wrote:
>
> 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
>
> _______________________________________________
> 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/19e10705/attachment-0004.html
More information about the vtigercrm-developers
mailing list