Public Variable Lost When Calling Program 1

[mdav2]

I have a public variable define in my start program that is used to identify the user in the system. When I run a call to a program from a screen it loses the value but when the program returns to the screen the value is repopulated.

This is the code that calls the program:

DO dorscoursequeue_insert WITH ;
						"0", ;
						THISFORM.cCourseref, ;
						CTOT(ALLTRIM(DTOC(TTOD(coursedate)))+ " " + ALLTRIM(coursetime) + ":00"), ;
						lcinstructsurname, ;
						lcinstructfirstnames, ;
						venuecap, ;
						dorssiteid, ;
						lnforcecontractid, ;
						{}, ;
						1, ;
						" ", ;
						gcUser, ;
						{}, ;
						"A", ;
						THISFORM.cCourseref

This is the program code:

LPARAMETERS ;
	tccourseid, ;
	tccoursetitle, ;
	tccoursedatetime, ;
	tcsurname, ;
	tcfirstnames, ;
	tncapacity, ;
	tndorssiteid, ;
	tnforcecontractid, ;
	tddummy1, ;
	tnsequence, ;
	tciscurrent, ;
	tcupuser, ;
	tddummy2, ;
	tcoperation, ;
	tcwcccourseid

* Create Insert Statement
lcsql = "INSERT INTO DorsCourseQueue ("
lcsql =lcsql + "CourseID, "
lcsql =lcsql + "CourseTitle, "
lcsql =lcsql + "CourseDateTime, "
lcsql =lcsql + "TutorForename, "
lcsql =lcsql + "TutorSurname, "
lcsql =lcsql + "CourseCapacity, "
lcsql =lcsql + "SiteID,"
lcsql =lcsql + "ForceContractID, "
lcsql =lcsql + "QueueTime, "
lcsql =lcsql + "Operation, "
lcsql =lcsql + "WccCourseID, "
lcsql =lcsql + "Updated, "
lcsql =lcsql + "Sequence, "
lcsql =lcsql + "IsCurrent, "
lcsql =lcsql + "UpUser) "
lcsql =lcsql + "VALUES ("
lcsql =lcsql + "'" + tccourseid       + ", "
lcsql =lcsql + "'" + tccoursetitle    + "', "
lcsql =lcsql + sqldate(tccoursedatetime) + ", "
lcsql =lcsql + "'" + tcfirstnames     + "', "
lcsql =lcsql + "'" + tcsurname        + "', "
lcsql =lcsql + "'" + ALLTRIM(STR(tncapacity))        + "', "
lcsql =lcsql + "'" + ALLTRIM(STR(tndorssiteid))      + "', "
lcsql =lcsql + "'" + ALLTRIM(STR(tnforcecontractid)) + "', "
lcsql =lcsql + " getdate(), "
lcsql =lcsql + "'" + tcoperation   + "', "
lcsql =lcsql + "'" + tcwcccourseid + "', "
lcsql =lcsql + "getdate(), "
lcsql =lcsql + "'" + ALLTRIM(STR(tnsequence)) + "', "
lcsql =lcsql + "'" + tciscurrent + "', "
lcsql =lcsql + "'" + tcupuser + "')"

IF odata.sql_exec(lcsql) <> .T.
	CREATEOBJECT("uError", PROGRAM(), LINENO(), gcUser, lcsql, gcUnique )
	RETURN .F.
ENDIF

The public variable lost is gcUser. It is lost as soon as the control transfers into the program. When the program completes and returns to the screen the value is back. Anyone have any ideas on why this would happen?

Mark Davies
Warwickshire County Council

 

[GriffMG]

You will probably be beaten up now for even discussing global variables - the use of them is considered shameful and beneath contempt these days!

I'm assuming you only need the gcUser for the error handling at the end of your procedure?

There are three things here to consider though...

Firstly, this isn't a procedure really, because it is expected to return a value (.f.) if the sql call fails so it should probably be declared as a function - and called as such.

myResult = dorscoursequeue_insert( "0", ;
  THISFORM.cCourseref, ;
  CTOT(ALLTRIM(DTOC(TTOD(coursedate)))+ " " + ALLTRIM(coursetime) + ":00"), ;
  lcinstructsurname, ;
  lcinstructfirstnames, ;
  venuecap, ;
  dorssiteid, ;
  lnforcecontractid, ;
  {}, ;
  1, ;
  " ", ;
  gcUser, ;
  {}, ;
  "A", ;
  THISFORM.cCourseref)

Secondly, your RETURN is in the middle of a control section (the IF...ENDIF bit) and that always looks wrong to me you would be better (to my eye) to do something like this.

lFLAG = .T.
IF odata.sql_exec(lcsql) <> .T.
    CREATEOBJECT("uError", PROGRAM(), LINENO(), gcUser, lcsql, gcUnique )    
    lFLAG = .F.
ENDIF

RETURN(lFLAG)

Thirdly, and this is the bit that matters to you I suspect, the global variable isn't necessary here - you can just replace the gcUser with tcupuser as it is passed to the function/procedure.

Why is gcUser going out of scope when you enter the procedure - I don't actually know! It might have something to do with the way the parameters are being passed I would check to see how your SET UDFPARMS TO is set.

Good luck

Regards

Griff
Keep [Smile]ing

There are 10 kinds of people in the world, those who understand binary and those who don't.

 

[cbasoz]

Probably your simple problem is because of you failed to prefix your memory variables with m.

You have a problem in your code. It is a code that gets executed against SQL server, right? It is screaming 'come and hack my database'.
Instead of passing hard coded values like that you should use parameters. That way you would also don't have need to functions like sqldate() which I guess that does nothing more than converting a date/time to ODBC canonical format. And for better readability you could simply use text ... endtext. Part of your code would look like:

LPARAMETERS ;
    tccourseid, ;
    tccoursetitle, ;
    tccoursedatetime, ;
    tcsurname, ;
    tcfirstnames, ;
    tncapacity, ;
    tndorssiteid, ;
    tnforcecontractid, ;
    tddummy1, ;
    tnsequence, ;
    tciscurrent, ;
    tcupuser, ;
    tddummy2, ;
    tcoperation, ;
    tcwcccourseid
LOCAL lcInsert
TEXT TO m.lcInsert noshow
INSERT INTO DorsCourseQueue 
  (CourseID, CourseTitle, CourseDateTime, 
   TutorForename, TutorSurname,
   CourseCapacity, SiteID,ForceContractID, 
   QueueTime, Operation, WccCourseID, 
   Updated, Sequence, IsCurrent, UpUser) 
   VALUES 
  (?m.tccourseid, ?m.tccoursetitle, ?m.tccoursedatetime, 
   ?m.tcfirstnames, ?m.tcsurname, ?m.tncapacity, 
   ?m.tndorssiteid, ?m.tnforcecontractid,  
   getdate(), ?m.tcoperation, ?m.tcwcccourseid, 
   getdate(), ?m.tnsequence,?m.tciscurrent, ?m.tcupuser)
ENDTEXT

*SQLEXEC(m.yourHandle, m.lcInsert)

Cetin Basoz
MS Foxpro MVP, MCP

 

[mdav2]

I managed to sort this out; the problem was sorted out by declaring a local variable, setting that to equal the value of the global variable then passing the local variable to the program. Thanks for the other comments too which I would like to spend more time looking at if I had the time to do so.

Some background on this was the system was written as an internal system just using FoxPro tables. I wrote it back in 2003 so it’s quite old and should really be overhauled. I work in local government so there is little money around to change things that work unless you really have to (example being that we still have some FoxPro for DOS systems running that were written back in the early 1990’s. In fact a user had printing problems with such a system and the fix from a now retired colleague was to simply convert the DOS code into a VFP exe. I am not kidding either. That’s how bad things are where I work. Usual story we spend loads on infrastructure and hardly anything on software development in comparison *ends rant*).

I have had to change it because it needs to communicate with an external system. It was updated to work with SQL server tables by a contractor we hired in. To be fair he had only a couple of months part time on this to make the changes and so didn’t do anything fancy. We had another contractor to write a web site to run web services that does all the calls to the external database. I have yet to test this part of the system yet.

I am going through the system to correct all the issues that occur when it is run, prior to it going live. I have many issues with the way the system has been coded to work with SQL Server this but don't have the knowledge and more importantly the time to completely address all these. I just need to get the thing in and working.

The comments were very useful.

1) What should be used instead of public variables? Create objects instead and reference them? Store the values in tables and reference them from there? I think all the public variables used are probably just around the error logging class.

2) I understand the issue with SQL injection on the statements and ideally would like to change the base code to use parameters. Another part of the systems processing that deals with web service interaction does use parameters and so the side that is exposed to the internet should be secure. I have to say my knowledge around this issue is very, very limited and I should look up on this. Any suggestions on where to look would be appreciated.

3) The text\endtext is also a good idea, I hate having to use variables because they are just ridiculously hard to read. In our oracle based systems we used a textmerge to file but I suppose putting it into a variable is better. If I get time to go through this in the future again I will make these changes. I like to copy and past SQL statements from code where possible without having to delete all the annoying characters in a string build.

4) One of the things I though weren’t really used much anymore would be memory variables. Scatter and Gather memvar I though was very old school and I haven’t used code like that since FoxPro 2.6.

5) The SQL date function does just create the date in an expected format. There is a similar function not shown in this example that formats text strings to pass them with enclosed quotes. I modified this to deal with extra apostrophes which pops up in the usual fields like surname (EG O’Brian)


Mark Davies
Warwickshire County Council

 

[cbasoz]

1) IMHO a VFP code should have a single public variable and the rest of the variables should be local:

* main.prg
public oApp
oApp = createobject('myApp')
* ...


define class myApp as custom
 AppWideProperty = 'SomeProperty'
* properties and methods appwide such as:
* Procedure SetDataLocation
* ...
enddefine

2) SQL injection is a well known issue listed in top 10 developer pitfalls. Simply using parameters prevents the injection.

3) Yes text..endtext is especially good in that you can copy paste tested code from within SQL Server Management Studio query window. Plus easier to create batch of statements.

4) Probably you got me wrong. I didn't mean scatter/gather. Memory variables are an important part of the language and I have yet to see a single application that doesn't use memory variables. In your code:

DO dorscoursequeue_insert WITH ;
                    "0", ;
                    THISFORM.cCourseref, ;
    CTOT(ALLTRIM(DTOC(TTOD(coursedate)))+ " " + ALLTRIM(coursetime) + ":00"), ;
                        lcinstructsurname, ;
                        lcinstructfirstnames, ;
                        venuecap, ;
                        dorssiteid, ;
                        lnforcecontractid, ;
                        {}, ;
                        1, ;
                        " ", ;
                        gcUser, ;
                        {}, ;
                        "A", ;
                        THISFORM.cCourseref

coursedate, coursetime, lcinstructsurname, lcinstructfirstnames, venuecap, dorssiteid, lnforcecontractid, gcUser

may or may not be memory variables. They can be field variables (and then would take precedence) or memory variables. Like I can't know, VFP doesn't either and check the fields in the current work area first. If it say finds a field named 'lcinstructsurname' then decides it IS a field variable and uses the current record's lcinstructsurname field content as the value. BUT if you explicitly alias them then VFP woudln't be confused at all:

m.lcinstructsurname

means it IS a memory variable whether there is a field named 'lcinstructsurname' or not. Hungarian notation or any other naming convention cannot prevent you, adding m. does. It also adds some speed too (maybe just a few ticks that wouldn't be important if it is not done in a loop).

5) There isn't really an expected format for dates in SQL server. SQL server can recognize many string representations of a date and most commonly ODBC canonical format is used for a universal date representation yyyy/mm/dd hh:MM:ss. Quoting the texts is another problem. All of these are not needed at all when you use parameters.

Cetin Basoz
MS Foxpro MVP, MCP

 

[mdav2]

I have a question with regards to SQL injection and the text\endtext. but I'll post that in a new topic

Mark Davies
Warwickshire County Council

 

[vfpdude]

Why not use the _screen object that is available even if the VFP screen is off (not visible)?

_screen.addproperty("cUser","Steve") will create the cUser property and store the user name. This property is then universally available at any point in the application and won't go out of scope.

 

[mdav2]

Thanks cbasoz for the detailed response. I agree that I should be creating an object and assigning any "global" values to that rather than declaring public variables. The application was written a long time ago now (back in VFP6 days) and changing it would take more time in re-testing etc. This is work the customer would not want to pay for. Ironically most of the problems I have found were changing a contractors code to work including SQL injection issues, issues around passing paramters and reworking the SQL statements. I got this all sorted and it has been a useful excercise for me.

Mark Davies
Warwickshire County Council