[Vtigercrm-developers] A rant about Error Checking, Lack Of in the CRM
clement chazarra
chazarra.clement at gmail.com
Fri Jan 12 07:24:53 PST 2007
I should also suggest that if a project like this is set up, we would need a
unique and available referent from vTiger team focusing on this project
which we could contact easily and efficiently.
On 1/12/07, clement chazarra <chazarra.clement at gmail.com> wrote:
>
> 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/728ecdf3/attachment-0004.html
More information about the vtigercrm-developers
mailing list