View Issue Details

Jump to NotesJump to History
This bug affects 1 person(s).
 6
IDProjectCategoryView StatusDate SubmittedLast Update
04420Bug reportsSurvey takingpublic2010-06-14 23:122010-07-20 22:16
Reporterjdalegonzalez Assigned ToEvan  
PrioritynormalSeverityminor 
Status closedResolutionfixed 
Product Version1.87+ 
Fixed in Version1.90RC2 
Summary04420: group.php will strip out {QUESTION_ESSENTIALS} if it appears at the very first part of the template
Description

The problem is on line 1046. The code is:

if( (strpos( $question_template , '{QUESTION_ESSENTIALS}') == false) || (strpos( $question_template , '{QUESTION_CLASS}') == false) )

Since it's using double = (==) and not triple equal (===) 0 and false will be equivalent and the if test will pass. Since the purpose of the code is to check if it exists in the template at all, the code should be using ===

Also, the code is testing both QUESTION_ESSENTIALS and QUESTION_CLASS but only removing QUESTION_ESSENTIALS. What that means is that if QUESTION_ESSENTIALS isn't in the template, but QUESTION_CLASS is in the template, the class will be duplicated. It will be in once in an outer div that group.php puts in and once from the template itself.

Steps To Reproduce

look at line 1046 (or create a template with {QUESTION_CLASS} at position 0 and {QUESTION_ESSENTIALS} somewhere else.

Additional Information

This is not really a big deal because the HTML that would be produced with {QUESTION_CLASS} or {QUESTION_ESSENTIALS} in position 0 would be syntactically invalid - so nobody is likely to do it.

TagsNo tags attached.
Bug heat6
Complete LimeSurvey version number (& build)7987
I will donate to the project if issue is resolved
Browserall
Database type & versionall
Server OS (if known)all
Webserver software & version (if known)all
PHP Versionall

Users monitoring this issue

There are no users monitoring this issue.

Activities

c_schmitz

c_schmitz

2010-06-17 00:52

administrator   ~12240

Evan, can you please have a look at this?
Evan

Evan

2010-07-01 04:03

reporter   ~12319

Last edited: 2010-07-01 04:05

Hi jdalegonzalez

{QUESTION_CLASS} should also be deleted if either {QUESTION_CLASS} or {QUESTION_ESSENTIALS} are missing.

Also when I looked at your observation of the bad {QUESTION_CLASS} or {QUESTION_ESSENTIALS} are at position 0. It seems the only way to make this behave reliably is to use preg_match() instead of strpos().

One last thing. in templatereplace() (common.php) All those strpos() calls followed by str_replace() to populate the templates are redundant. I think they should be removed as str_replace won't give an error if there is match for the find string, it will just return the original string unaltered. This should speed up the templatereplace somewhat.

I commit a fix including removing the strpos() calls (where appropriate) tonight (Sydney, Australia time).
jdalegonzalez

jdalegonzalez

2010-07-01 21:57

reporter   ~12329

Hey Evan, I think you can use triple equals with false (strpos() === false v. strpos() == false) and the return of strpos and get what you're looking for. With the triple equals, the number and type (boolean) have to match. A 0 (ie first position in the string) won't match false. With the double, it will. (I don't know why I just explained that to you, you probably already knew it.)

However replacing with preg_match is probably fine too. Small performance hit but it's only one call per page display. I love the "remove the strpos" calls idea. I should have spotted that.

Thanks for looking at this. You guys are great.
Evan

Evan

2010-07-07 21:03

reporter   ~12375

used preg_match rather than strpos to check if {QUESTION_ESSENTIALS} is present. Also now removes {QUESTION_CLASS} if {QUESTION_ESSENTIALS} is missing. applies to question.php, group.php and survey.php

Issue History

Date Modified Username Field Change
2010-06-14 23:12 jdalegonzalez New Issue
2010-06-14 23:12 jdalegonzalez Status new => assigned
2010-06-14 23:12 jdalegonzalez Assigned To => user372
2010-06-17 00:52 c_schmitz Assigned To user372 => Evan
2010-06-17 00:52 c_schmitz Note Added: 12240
2010-07-01 04:03 Evan Note Added: 12319
2010-07-01 04:03 Evan Status assigned => feedback
2010-07-01 04:05 Evan Note Edited: 12319
2010-07-01 21:57 jdalegonzalez Note Added: 12329
2010-07-01 21:57 jdalegonzalez Status feedback => assigned
2010-07-07 21:03 Evan Note Added: 12375
2010-07-07 21:03 Evan Status assigned => resolved
2010-07-07 21:03 Evan Fixed in Version => 1.90RC2
2010-07-07 21:03 Evan Resolution open => fixed
2010-07-20 22:16 c_schmitz Status resolved => closed
2010-10-25 00:18 c_schmitz Category Survey at Runtime => Survey taking