Hi guys,<br><br>I am not a really skilled developer, but I would like as well to see vTiger evolve to a product even more stable.<br><br>As
Dennis Grant, Richie and many others are outlining, there are many issues in the source code design, security, tests, etc.<br>My point of view is if we try to fixes those issues without a structured process, it will never end..
<br><br>I would suggest:<br>1. Categories the needs of softwares in general on a Wiki: <br> Security / List of the possible hacks for PHP Mysql Apache etc.<br> Code efficiency / No duplicated code, no dead code, etc.
<br> Code design / Variables on top, functions written clearly and in logic order, functions variables commented<br> User interface / Display, form verification, Ajax<br> Add other categories or modify those above, I just give an overview of what I think
<br><br>2. Once we have categories, define the rules on a Wiki:<br> Security / Fix SQL injections: Use the function xxx in every sql queries<br> Fix ...<br> Code efficiency / Delete all the code commented
<br> Delete all the code not used<br> Define main design patterns precisely<br> Recode each functions which can be improved
<br> Code design / Comment each variables<br> / Comment each function<br> / Create complete vTiger API<br> User interface / Manage and fix design problems
<br> / Manage and fix Ajax<br> / Manage and fix Javascript verifications on forms<br> Add other categories or modify those above, I just give an overview of what I think
<br><br>3. Wait for the release of vTigercrm-5.0.3<br><br>4. Create a Branche in vTiger Trac system named "5.0.3 Core Code Improvement"<br><br>5. Work on files 1 by 1 according to the categories and rules<br><br>
6. When file has been treated in this process once, a second check must be performed<br><br>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.
<br><br>What do you think?<br><br>Regards,<br>Clem<br><br><br><div><span class="gmail_quote">On 1/12/07, <b class="gmail_sendername">Joe Bordes</b> <<a href="mailto:joe@tsolucio.com">joe@tsolucio.com</a>> wrote:</span>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Hi Richie,<br><br>I would really like to help. But my experience has been that you don't
<br>want that help.<br><br>I have asked many "developer" questions on this list and have received<br>no answer. I am adding modules and code to vtiger and need to know the<br>changes so these can keep up<br>I have directly asked questions about vforge to no avail
<br>I have sent email thru web asking to colaborate and host <a href="http://vtiger.es">vtiger.es</a> -><br>nothing<br>I have offered my help with exact details -> nothing<br>I was actively helping in the forums and wiki -> doesn't to seem
<br>important<br><br>Sorry to be so harsh, it is not my intention (email is very cold). I am<br>just stating my personal experience and feelings. If you can get in<br>contact with me, I am very interested in establishing a relationship
<br>with VTiger and helping out.<br><br>Regards, Joe<br>TSolucio<br><br>El vie, 12-01-2007 a las 14:47 +0530, richie escribiσ:<br>> Dennis you are bang on target.<br>> Yes, we do have inconsistencies of the kind you have mentioned and
<br>> some worse too ;-).<br>><br>> Thanks for pointing these out.<br>> If you have made some fixes to these kinds of issues, do contribute<br>> the same back and we will have it integrated into the core.<br>
><br>> The code can be vastly improved team and we need your help. This is<br>> something that I have always asked for. We are not geniuses but we are<br>> willing to learn.<br>> Yes there are pretty, pretty bad practices in the code but then not
<br>> all the code is junk. Together we can clean it up much much faster.<br>><br>> Richie<br>><br>> Dennis Grant wrote:<br>> > OK gang, time for me to go off on a rant here:<br>> ><br>> >
<br>> ><br>> > I recently had a ticket where a user a manager was complaining<br>> > about how the HelpDesk-TroubleTickets module had two views called<br>> > "All", and they had different definitions.
<br>> ><br>> ><br>> ><br>> > I had a look inside the vtiger_customview table, and sure enough,<br>> > there were two rows in HelpDesk titled "All"; one with a viewed of<br>> > "64" and one with a viewed of '95".
<br>> ><br>> ><br>> ><br>> > I was able to determine that viewed = 64 was the correct one. It<br>> > looked to me like somebody had created a custom view titled<br>> > "All" (that ought to be a reserved word and should be rejected as a
<br>> > possible custom view name) and the user interface (soundly) doesn't<br>> > allow a view named "All" to be deleted.<br>> ><br>> ><br>> ><br>> > So I deleted that row from the database and all hell broke loose.
<br>> > All of a sudden, I couldn't view the ListView for the HelpDesk<br>> > module any more; instead, I got the familiar "PearDatabase" error<br>> > which means some Sql statement somewhere is screwed up.
<br>> ><br>> ><br>> ><br>> > So I go "whoops!" and I put the line back in the table and that<br>> > DOESN'T FIX THE PROBLEM.<br>> ><br>> ><br>> ><br>> > Oh. Shit.<br>
> ><br>> ><br>> ><br>> > It takes me a few seconds to work out that something, somewhere has<br>> > associated my user with the cvid of 95 and my yanking it out has<br>> > broken that association. I go looking for where that association is
<br>> > stored (FOR USER admin MODULE helpdesk USE LISTVIEW CUSTOMVIEW 95)<br>> > but I can't find it.<br>> ><br>> ><br>> ><br>> > So I go into the code itself, and discover the culprit is the line:
<br>> ><br>> ><br>> ><br>> > $list_query = $oCustomView->getModifiedCvListQuery($viewid,<br>> > $listquery,"HelpDesk");<br>> ><br>> ><br>> ><br>> > In HelpDesk/ListView.php. $list_query is coming back as "select ,
<br>> > foo bar blech", and then later on that query is executed. This query<br>> > not being proper SQL syntax, it bombs.<br>> ><br>> ><br>> ><br>> > As a quick fix, I add the following hack:
<br>> ><br>> ><br>> ><br>> > If ($viewid == 95) {<br>> ><br>> > $viewid = 64;<br>> ><br>> > }<br>> ><br>> ><br>> ><br>> > Which restores functionality whew! and then I go looking to see
<br>> > what the hell happened.<br>> ><br>> ><br>> ><br>> > I am not a DBA; my database experience up to this point was a couple<br>> > of basic SQL and database classes in college, and some basic
<br>> > database reporting apps in the 20-odd years since. I'm learning the<br>> > DBA stuff on the fly. I was not aware of "ON DELETE CASCADE" foreign<br>> > key constraints, and the concept that deleting a row from one table
<br>> > might cascade through other tables. I am now. I discovered that<br>> > there are a number of tables set up that use the vtiger_customview<br>> > table as a foreign key, and have the ON DELETE CASCADE constraint on
<br>> > them. Thus, deleting the row from vtiger_customview deleted rows<br>> > from vtiger_cvcolumnlist (and a few others) which explains why my<br>> > putting that row back in didn't fix the problem. I guess I need to
<br>> > learn about rollbacks.<br>> ><br>> ><br>> ><br>> > That's *MY* mistake. It is always dangerous to muck around with the<br>> > brains of an application without knowing all the ramifications
<br>> > beforehand. Something innocent looking turned out to have deeper<br>> > implications than I assumed, and I got myself into a spot of<br>> > trouble. My Bad.<br>> ><br>> ><br>> >
<br>> > But and this is what I want to rant about there is blame to<br>> > share here. Because in further debugging the chain of events that<br>> > led to a PHP error being displayed to a user, one sees that the CRM
<br>> > code is making all kinds of assumptions about its data. The<br>> > application is positively RIFE with:<br>> ><br>> ><br>> ><br>> > $sql_query = buildQueryDynamically();<br>> >
<br>> > $result_set = sql->execute($sql_query);<br>> ><br>> > doSomething($result_set);<br>> ><br>> ><br>> ><br>> > Where NONE of these procedures EVER checks for errors.<br>> >
<br>> ><br>> ><br>> > Here's an actual example:<br>> ><br>> ><br>> ><br>> > function getCvColumnListSQL($cvid)<br>> ><br>> > {<br>> ><br>> > $columnslist = $this->getColumnsListByCvid($cvid);
<br>> ><br>> > if(isset($columnslist))<br>> ><br>> > {<br>> ><br>> > foreach($columnslist as $columnname=>$value)<br>> >
<br>> > {<br>> ><br>> > $tablefield = "";<br>> ><br>> > if($value != "")<br>> >
<br>> > {<br>> ><br>> > $list = explode(":",$value);<br>> ><br>> ><br>> ><br>> > //Added For getting status
<br>> > for Activities -Jaguar<br>> ><br>> > $sqllist_column =<br>> > $list[0].".".$list[1];<br>> ><br>> > if($this->customviewmodule
<br>> > == "Calendar")<br>> ><br>> > {<br>> ><br>> > if($list[1] ==<br>> > "status")
<br>> ><br>> > {<br>> ><br>> ><br>> > $sqllist_column = "case when<br>> > (vtiger_activity.status not like '') then vtiger_activity.status
<br>> > else vtiger_activity.eventstatus end as activitystatus";<br>> ><br>> > }<br>> ><br>> > }<br>
> ><br>> ><br>> ><br>> > //Added for for assigned to<br>> > sorting<br>> ><br>> > if($list[1] == "smownerid")
<br>> ><br>> > {<br>> ><br>> > $sqllist_column =<br>> > "case when (vtiger_users.user_name not like '') then
<br>> > vtiger_users.user_name else vtiger_groups.groupname end as<br>> > user_name";<br>> ><br>> > }<br>> ><br>> ><br>> ><br>> > $sqllist[] =
<br>> > $sqllist_column;<br>> ><br>> > //Ends<br>> ><br>> ><br>> ><br>> > $tablefield[$list[0]] =<br>
> > $list[1];<br>> ><br>> > $fieldlabel =<br>> > trim(str_replace($this->escapemodule," ",$list[3]));<br>> ><br>> ><br>> > $this->list_fields[$fieldlabel] = $tablefield;
<br>> ><br>> ><br>> > $this->list_fields_name[$fieldlabel] = $list[2];<br>> ><br>> > }<br>> ><br>> > }<br>> ><br>> > $returnsql = implode(",",$sqllist);
<br>> ><br>> > }<br>> ><br>> > return $returnsql;<br>> ><br>> ><br>> ><br>> > }<br>> ><br>> ><br>> ><br>> > This function assumes everything works there's no test to see if
<br>> > the $cvid parameter passed in is not null. It checks to see if<br>> > $columnlist isset before doing stuff, but has no code in the case of<br>> > $columnlist NOT being set. There's no test to check for $columnlist
<br>> > being set, but containing no rows. It COULD attempt to fill in<br>> > reasonable defaults. It COULD flag the error back higher to the<br>> > function that called it. There's a million different things that it
<br>> > COULD do but instead, it fails silently, returning bogus data,<br>> > which breaks the upstream code.<br>> ><br>> ><br>> ><br>> > This function is only half done. It works for cases where everything
<br>> > works as expected, but fails spectacularly in cases where the<br>> > unexpected happens:<br>> ><br>> ><br>> ><br>> > - what should it do if $columnslist fails the isset test?<br>
> ><br>> > - what should it do if $columnname or $value is NULL?<br>> ><br>> > - what should it do if the explode() fails (meaning that the data in<br>> > value is not in the expected format)?
<br>> ><br>> ><br>> ><br>> > And this sort of thing happens ALL OVER THE PLACE in the CRM code.<br>> > There is a persistent and pathological lack of error checking and<br>> > handling.
<br>> ><br>> ><br>> ><br>> > This needs to be rectified.<br>> ><br>> ><br>> ><br>> > Please, please, PLEASE if you are writing new code for the CRM,<br>> > take the time to do the error checking and handling!
<br>> ><br>> ><br>> ><br>> > DG<br>> ><br>> ><br>> ><br>> > ____________________________________________________________________<br>> ><br>> > _______________________________________________
<br>> > Reach hundreds of potential candidates - <a href="http://jobs.vtiger.com">http://jobs.vtiger.com</a><br>><br>> _______________________________________________<br>> Reach hundreds of potential candidates -
<a href="http://jobs.vtiger.com">http://jobs.vtiger.com</a><br><br>_______________________________________________<br>Reach hundreds of potential candidates - <a href="http://jobs.vtiger.com">http://jobs.vtiger.com</a> </blockquote>
</div><br>