LimeSurvey issue tracker
Registration

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
05268User patchesTemplatespublic2011-06-12 10:352012-06-21 13:22
ReporterTMSWhite 
Assigned Toc_schmitz 
PrioritynormalSeverityminor 
StatusclosedResolutionfixed 
Product Version1.90+ 
Target VersionFixed in Version 
Summary05268: Do all LimeReplacementField and Token replacements in a single function
DescriptionToken and LimeReplacementField replacements are currently managed by str_replace() and ReplaceFields() calls spread throughout the code.

Although there are about 100 LimeReplacementFields (plus the question-specific ones), there is no consolidated place for managing them - either the list of "reserved words" they represent, or their values. This could make adding new ones and regression testing cumbersome.

The expression parser built to solve Issues 05103 can now support both equations, and also any existing LimeReplacementField, Token, or SGQA code (see attached patch).

So, I propose:
(1) create a consolidated function for the many LimeReplacementField translation calls (e.g. str_replace() and the $fieldsarray[] statements used by ReplaceFields()) that returns an array of all LimeReplacementField variable names and their current values.
(2) Pass that array to ExpressionManager, which will then handle the replacements for all Questions, Answers, etc. (any text that needs to be generated)
(3) Remove the obsolete code (including the classes/dTexts module, whose functionality is already replicated by ExpressionManager)

In order to do this, I need to understand:
(1) in how many different places such replacement currently occurs
(2) whether that will be changing with CI
(3) whether there are different degrees of "global" values for those fields so that I don't reset the array fully each time. I expect there are:
(a)system-wide variables
(b)survey-wide variables
(c)group-specific variables (e.g. constant for the active group)
(d)question-specific variables
(e)sub-question-specific ones
(f)answer-specific ones
(g)user session-specific variables.
Steps To ReproduceYou can view the Unit tests of Expression Manager here:

http://localhost/limesurvey/classes/eval/Test_ExpressionManager_Evaluate.php [^]

http://localhost/limesurvey/classes/eval/Test_ExpressionManager_ProcessStringContainingExpressions.php [^]
TagsNo tags attached.
LimeSurvey build number OR git commit ID10256
Attached Filespatch file icon issue_05103-rev2.patch [^] (83,074 bytes) 2011-06-12 10:35 [Show Content]
patch file icon issue_05268-20110616.patch [^] (140,253 bytes) 2011-06-17 00:22
patch file icon issue_05268-20110617.patch [^] (153,393 bytes) 2011-06-17 11:57
patch file icon issue_05268-20110617-v2.patch [^] (155,683 bytes) 2011-06-18 00:36

- Relationships
related to 05103closedTMSWhite User patches Support conditional piping/tailoring and complex calculations via embedded equation parser 
related to 05288closedTMSWhite User patches Optionally replace Assessments with ExpressionManager features 
related to 05269closedTMSWhite User patches Use ExpressionManager for Branching logic as optional alternative to Conditions 
related to 05279acknowledged Development  Add a GUI for ExpressionManager 
related to 05317closedTMSWhite Bug reports templatereplace() doesn't properly handle some missing replacement values 

-  Notes
User avatar (15413)
lemeur (administrator)
2011-06-12 18:02

Argh.. I prepared a full answer during 40 minutes and this F..cking Mantis decided I was to slow and refused my post...........................................................
User avatar (15414)
lemeur (administrator)
2011-06-12 18:10

Let's try to rewrite this...

I agree on your 3 proposals, I think this is the way to go.

Your questions:
(1) I'm afraid that no one has the big picture in his mind, just because these features result from contributions from different people who hadn't the big picture in mind themselves... Personnaly I know that I don't have the big picture in mind, but I'll try a guess.

In common_functions.php, the templatereplace function is used to replace template-specific variables (survey title, welcome text, ...). It takes substitution variables from the array you pointed at in the ticket's description

Then it calls insertAnsReplace (for previous answers substitution), tokenReplace for token-specific substitutions.

There is the PassthruReplace which replaces variables in the URL, but It's the first time I see this one.

There is the ReplaceFields function which is used for substitution inside email templates.

(2) Yes, It would be surprizing if porting to CI wouldn't change this

(3) Yes, we must sort these variables but as I said no one already did this. Let's use this ticket for this purpose.
User avatar (15420)
TMSWhite (reporter)
2011-06-13 18:50

Should have asked this question here...

What subset of classes/dTexts/* is operations?

The {INSERTANS:xxxx} function (dTexts/dFunctions/dFunctionInsertAns.php) is clearly used. (although the insertansReplace() function is called by ReplaceFields() from submittokens() - so dTexts is one of two active ways of dealing with {INSERTANS:xxxx})

Although dTexts has support for {TOKEN:xxxx} (dTexts/dFunctions/dFunctionToken.php), it isn't clear whether that is actually used, since {TOKEN:xxxx} replacement also happens in templatereplace(), which calls tokenReplace().

However, what about the other ones? Are any of them used, and if so, where can I find documentation and what they are supposed to do?

{HIDE:xxxx}
{IF:xxxx}
{IFCOUNT:xxxx}
{IFIN:xxxx}
{SWITCH:xxxx}


Based upon lemeur's note above, sounds like dTexts can be made obsolete, so I should focus just on the templatereplace() function.
User avatar (15421)
TMSWhite (reporter)
2011-06-13 19:02

LimeSurvey appears to support recursive substitution of LimeReplacementFields. Is that the desired behavior?

Given the heavy, and sequential use of str_replace(), if a replacement value has an embedded LimeReplacementField, a subsequent step might replace it (but this would be brittle as a user would have to know the order in which replacements are done).

Is this the desired behavior, or a potential security hole?
User avatar (15422)
TMSWhite (reporter)
2011-06-13 20:02

It looks like it should be safe to modify the following functions in common_functions.php

TemplateReplace() -- replace all str_replace calls with create a replacementField, then call ReplaceFields()
ReplaceFields() -- have it call ExpressionManager, passing in the replacementFieldValues
TokenReplace() -- have it create ReplacementFields for each valid token, then call ReplaceFields()
insertAnsReplace() -- have it create ReplacementFields for each valid INSERTANS: field, then call ReplaceFields()
PassthroughReplace() -- have it create ReplacementFields for each valid Passthrough (I'll need to understand how {PASSTHRU:myarg} is supposed to work), then call ReplaceFields()
getQuotaInformation() -- have it create ReplacementFields, then call ReplaceFields()

There are some other functions that do str_replace(), but there are so few parameters, that if I start with common_function.php, we should cover 90+% of the cases.
User avatar (15426)
TMSWhite (reporter)
2011-06-14 00:08

Also, I'd need to:
(1) Replace any call to dTexts:run()(e.g.for INSERTANS replacement
(2) Create retrieve_answers(array $varnames). It would be like retrieve_answer(), but would mapping of $ia => answer value for all variables in the current group in a single SQL query (rather than issuing a query per variable as is done by retrieve_answer).

Is there an alternate way to retrieve the current answer values?

Is there any reason why LimeSurvey can't retain an array of current answer values for all variables for the duration of the session, updating them locally, and also persisting them to the database?
User avatar (15452)
TMSWhite (reporter)
2011-06-16 09:10

Most recent patch in http://bugs.limesurvey.org/view.php?id=5103 [^] starts to address this issue. There is a test survey of all question types showing how named variables can be used for reporting of question, response value, and response code (in additional to the usual conditional substitution).

That patch is just integrated into template_replace, but it should not be hard to integrate it in the other places that need it.
User avatar (15456)
TMSWhite (reporter)
2011-06-16 14:15

I've posted detailed instructions and screen shots here: http://bugs.limesurvey.org/view.php?id=5103 [^] for those who want to test the ExpressionManager
User avatar (15471)
TMSWhite (reporter)
2011-06-17 00:27

Please try patch issue_05268-20110616.patch (e.g. apply it against the main LS1 branch).

It is a brute-force replacement of str_replace() functions in common_functions.php (see above).

It appears to be working, except for {NUMBEROFQUESTIONS}, which per the in-line comments, has to be dealt with on a second pass anyway.

I don't have any experience working with Tokens or other templates, so if someone could test the functionality with both of those (e.g. just test a bunch of surveys), I'd appreciate it.

As time permits, I'll work on a more elegant solution to dealing with the list of replacement fields. Right now, I've commented out the relevant sections of templatereplace() and other functions within common_functions.php. It would be better, of course, to just have a single function that generates those replacement values, and calls a small function to determine the proper string value as needed (rather than having a lot of if-then statements within templatereplace()).
User avatar (15473)
TMSWhite (reporter)
2011-06-17 11:59

Refactored templatereplace() - it is now using exclusively ExpressionManager

Tested and fixed all known bugs in previous patch (esp. /limesurvey/index.php not working, and various MYSQL errors)
{NUMBEROFQUESTIONS} is also working.

So, please use this patch when testing ExpressionManager
User avatar (15484)
TMSWhite (reporter)
2011-06-18 00:36

Have now tested ExpressionManager integration into LimeSurvey for the following features:
(1) Development + deployed surveys
(2) Token management
(3) Template management
(4) Data entry screen
(5) Printable summary of responses given - passed this through templatereplace() so that can see the text as the respondent saw it, not as the survey-designer built it.

In the process, identified 1 small bug in ExpressionManager, and several bugs in other files (mostly lack of detection of undefined variables).

The only sections I have yet to test are ones that aren't built into any of my test surveys:
(1) Assessments
(2) Conditions

Please try the latest patch (issue_50268-20110617-v2.patch)
User avatar (15486)
TMSWhite (reporter)
2011-06-18 04:14

The Keywords section of the documentation (http://docs.limesurvey.org/The+template+editor&structure=English+Instructions+for+LimeSurvey [^]) specifies which keywords are valid for which .pstpl files.

Currently, the redesign of templatereplace() pre-sets all 72 of the possible keywords, regardless of white template is being called.

Should I refactor this so that only keywords that are valid for the specified templates are passed through to ExpressionManager?
User avatar (15487)
lemeur (administrator)
2011-06-18 08:47

Hi Tom,

About post 14486, I don't think it matters if you preset keywords value for fields that are not relevant to the specific template file.

I guess that you preset the keyword to an empty value if it cannot be eveluated at the time the template file is used (for instance a question's answer in the Welcome page)? Am I right ?
User avatar (15492)
TMSWhite (reporter)
2011-06-18 14:11

Lemeur-

Right now, I do pre-set those non-applicable keywords to blank, so if someone tries to use a non-applicable keyword on a template, it will just be invisible.

If you would like, we could, instead, have use of non-applicable keywords show up as an error - e.g. the keywords would be highlighted and boxed in red.
User avatar (15495)
lemeur (administrator)
2011-06-18 18:38

Tom,

The current behaviour is ok to me.
I've assigned the Ticket to Carsten so that he can give us his opinion on such decisions.

Thibault
User avatar (16800)
TMSWhite (reporter)
2012-01-16 16:12

This was fixed in 1.91+ and beyond - replacements.php

- Issue History
Date Modified Username Field Change
2011-06-12 10:35 TMSWhite New Issue
2011-06-12 10:35 TMSWhite File Added: issue_05103-rev2.patch
2011-06-12 18:02 lemeur Note Added: 15413
2011-06-12 18:10 lemeur Note Added: 15414
2011-06-13 18:50 TMSWhite Note Added: 15420
2011-06-13 19:02 TMSWhite Note Added: 15421
2011-06-13 20:02 TMSWhite Note Added: 15422
2011-06-14 00:08 TMSWhite Note Added: 15426
2011-06-16 09:10 TMSWhite Note Added: 15452
2011-06-16 14:15 TMSWhite Note Added: 15456
2011-06-17 00:22 TMSWhite File Added: issue_05268-20110616.patch
2011-06-17 00:27 TMSWhite Note Added: 15471
2011-06-17 11:57 TMSWhite File Added: issue_05268-20110617.patch
2011-06-17 11:59 TMSWhite Note Added: 15473
2011-06-18 00:36 TMSWhite Note Added: 15484
2011-06-18 00:36 TMSWhite File Added: issue_05268-20110617-v2.patch
2011-06-18 04:14 TMSWhite Note Added: 15486
2011-06-18 08:43 lemeur Assigned To => c_schmitz
2011-06-18 08:43 lemeur Status new => assigned
2011-06-18 08:47 lemeur Note Added: 15487
2011-06-18 14:11 TMSWhite Note Added: 15492
2011-06-18 18:38 lemeur Note Added: 15495
2011-06-19 18:59 TMSWhite Relationship added related to 05288
2011-06-19 19:01 TMSWhite Relationship added related to 05269
2011-06-19 19:02 TMSWhite Relationship added related to 05103
2011-06-19 19:04 TMSWhite Relationship added related to 05279
2011-07-03 06:48 TMSWhite Relationship added related to 05317
2012-01-16 16:12 TMSWhite Note Added: 16800
2012-01-16 16:12 TMSWhite Status assigned => resolved
2012-01-16 16:12 TMSWhite Resolution open => fixed
2012-06-21 13:22 c_schmitz Status resolved => closed


Copyright © 2000 - 2014 MantisBT Team
Powered by Mantis Bugtracker