View Issue Details

IDProjectCategoryView StatusLast Update
12613Bug reports[All Projects] Survey takingpublic2019-06-14 08:50
Reporterollehar Assigned Toollehar  
PrioritynoneSeveritymajor 
Status assignedResolutionopen 
Product Version2.67.x 
Target VersionFixed in Version 
Summary12613: Condition NAOK >= " " (no answer) evaluated differently in JS/PHP
Description

Title.

Possible solutions:

1) Use the tokenized expression and parse it using a small VM which can be copied to JS. Instead of generating JS code, we send the tokens to the client.

2) Use JavaScript on the server to make sure the evaluation is exactly the same as on the client

Steps To Reproduce

Activate attached survey, run it, answer for second question is not recorded in database.

TagsNo tags attached.
Complete LimeSurvey version number (& build)latest
I will donate to the project if issue is resolvedNo
Browser-
Database & DB-Version-
Server OS (if known)-
Webserver software & version (if known)-
PHP Version-

Relationships

related to 07805 closedDenisChenu Comparaison String and Numeric is different in same page and other page 

Activities

ollehar

ollehar

2017-08-21 10:47

administrator  

limesurvey_survey_563168.lss (14,275 bytes)
ollehar

ollehar

2017-08-21 12:01

administrator   ~44316

Problem: https://github.com/LimeSurvey/LimeSurvey/blob/master/application/helpers/expressions/em_core_helper.php#L373

ollehar

ollehar

2017-08-21 12:24

administrator   ~44317

GetJavaScriptEquivalentOfExpression() does not check for arguments type mismatch.

ollehar

ollehar

2017-08-21 12:27

administrator   ~44318

Here: https://github.com/LimeSurvey/LimeSurvey/blob/master/application/helpers/expressions/em_core_helper.php#L1323

ollehar

ollehar

2017-08-21 12:27

administrator   ~44319

@DenisChenu On vacation, still?

DenisChenu

DenisChenu

2017-08-21 12:32

developer   ~44320

Coming back now …

The didn't test for "arguments type mismatch" is needed. Because with single choice answer : it's a code, the "1" is a string … forcing to numeric if comparaison is done with a number

PHP compare is done here : https://github.com/LimeSurvey/LimeSurvey/blob/master/application/helpers/expressions/em_core_helper.php#L288

ollehar

ollehar

2017-08-21 12:33

administrator   ~44321

Care to join me on IRC?

DenisChenu

DenisChenu

2017-08-21 12:38

developer   ~44322

Eating before , come on irc this afternoon. I test with the survey

ollehar

ollehar

2017-08-21 12:39

administrator   ~44323

Great! :) A customer had this problem on LimeService, and I've confirmed it. We need to generate type mismatch code for the JS part too, not only PHP.

ollehar

ollehar

2017-08-21 12:39

administrator   ~44324

I will look into how to unit-test JavaScript together with PHPUnit.

DenisChenu

DenisChenu

2017-08-21 12:42

developer   ~44325

It's not empty, it's space … tehn there are a mismatch type with PHP => false , but return true in JS … only with space or with empty string too ?

ollehar

ollehar

2017-08-21 12:44

administrator   ~44326

The space is not a problem.

JS: 1 >= " " ---> true

PHP: false >= " " ---> false

Why "false" and not 1? Because of type mismatch check.

DenisChenu

DenisChenu

2017-08-21 12:53

developer   ~44327

JS: 1 >= "!" or 1 >= "A" or 1 <= "!" or 1 <= "A" ---> false (even if char code is lesser). Maybe a javascript in 2.6lts

See new lss http://demonstration.sondages.pro/563168?newtest=Y



limesurvey_survey_emptyMoreTest.lss (14,711 bytes)
DenisChenu

DenisChenu

2017-08-21 13:05

developer   ~44328

See Using the Equality Operators on https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Comparison_Operators

When comparing a number and a string, the string is converted to a number value. JavaScript attempts to convert the string numeric literal to a Number type value. First, a mathematical value is derived from the string numeric literal. Next, this value is rounded to nearest Number type value.

ollehar

ollehar

2017-08-21 13:16

administrator   ~44329

You're overthinking this. The type mismatch transform

1 >= " "

into

false >= " "

Dump in RDP_EvaluateBinary() for details.

ollehar

ollehar

2017-08-21 14:10

administrator   ~44330

Maybe more relevant: When constructing a condition, the empty string is used to represent "No answer".

ollehar

ollehar

2017-08-21 14:12

administrator  

Selection_263.png (17,190 bytes)
Selection_263.png (17,190 bytes)
Selection_262.png (10,116 bytes)
Selection_262.png (10,116 bytes)
DenisChenu

DenisChenu

2017-08-21 14:49

developer   ~44331

RDP_EvaluateBinary : result is an array : 0: the result, 1: don't remind and 2: the value type for EM next function

And we already have a specific fix for JS comparaison for le (i don't add it, surely since 1.92)
https://github.com/LimeSurvey/LimeSurvey/blob/master/application/helpers/expressions/em_core_helper.php#L347

DenisChenu

DenisChenu

2017-08-21 15:06

developer   ~44332

See another updated lss file. tested on 2.67.3
q1.NAOK >= "" : {q1.NAOK >= ""}<br>
q1.NAOK <= "" : {q1.NAOK <= ""}<br>
q1.NAOK == "" : {q1.NAOK == ""}<br>
q1.NAOK >= " " : {q1.NAOK >= " "}<br>
q1.NAOK <= " " : {q1.NAOK <= " "}<br>
q1.NAOK >= "A" : {q1.NAOK >= "A"}<br>
q1.NAOK >= "1A" : {q1.NAOK >= "1A"}<br>
q1.NAOK >= "!" : {q1.NAOK >= "!"}<br>
q1.NAOK <= "A" : {q1.NAOK >= "A"}<br>
q1.NAOK <= "1A" : {q1.NAOK >= "1A"}<br>
q1.NAOK <= "!" : {q1.NAOK >= "!"}<br>

Try with -1 on q1 : same error with " ", but seems there are an issue with "" (emty string) too … �



limesurvey_survey_emptyMoreMoreTest.lss (15,243 bytes)
ollehar

ollehar

2017-08-24 11:19

administrator   ~44338

Last edited: 2017-08-24 11:22

View 4 revisions

Some other approaches:

  1. Dump _SESSION for each user in JSON in database, BEFORE it's manipulated by the EM. This is to always have a fallback. New field in SurveyDynamic and table survey_xxx called "session_dump"?
  2. If relevance = 0 in PHP BUT we have session value, show error (we know there's a mismatch between PHP and JS)
  3. Send relevance eq to PHP using Ajax instead of generating JS.
DenisChenu

DenisChenu

2017-08-24 11:32

developer   ~44339

I think there are 2 bugs here :).

1st : no answer IS "" not a space (test with is-empty for example)
2nd : PHP vs JS comparaison : i'm totally unsure what JS do with -1 > "A" ; -1 < "A" … when fix the old issue : information on comment are 'js send always false if one is NUMBER and other is a string without numeric character.

ollehar

ollehar

2017-08-24 11:33

administrator   ~44340

  1. OK, then the condition designer if wrong, because it generate NAOK >= " " (space)
DenisChenu

DenisChenu

2017-08-24 11:44

developer   ~44341

  1. I think yes :)

PS : the test done is … strange … and there are no valide answer. -1 < no-answer or not ?
0 == no answer or not ?
etc …

ollehar

ollehar

2017-08-24 11:46

administrator   ~44342

Valid answer or not is not important. Only thing that matters is that PHP and JS does the same thing.

DenisChenu

DenisChenu

2017-08-24 11:50

developer   ~44343

Yes :).

1st part : make a complete test with pure javascript to test (for example) 1 < "A" , 1 < " ", 1 < "!" etc … if "number" : js cast it as string (sure at 99.9942%)
2nd part : validate when EM cast as number ( how to : usage of your test ?) think it's ONLY for value from user (and Equation) but unsure for function
3rd part : mimic js in PHP

What do you think of this step ?

ollehar

ollehar

2017-08-24 11:54

administrator   ~44344

Some input from stackoverflow: https://stackoverflow.com/questions/45858228/evaluate-binary-operations-in-both-php-and-javascript

ollehar

ollehar

2017-08-24 11:56

administrator   ~44345

-1 < 'A' ---> false in JavaScript

but

var v = -1
v.toString() < 'A' ---> true in JavaScript

ollehar

ollehar

2017-08-24 12:01

administrator   ~44346

About 1st part: That's basically what I'm doing already in my unit tests.

You think the problem is that JavaScript casts numbers to strings at certain points, and PHP does not?

ollehar

ollehar

2017-08-24 12:57

administrator   ~44347

Chapter about type coersion in JavaScript: https://www.safaribooksonline.com/library/view/you-dont-know/9781491905159/ch04.html

Another problem: EM in PHP does not know at evaluation if the function is onlynumber. Look at this:

"0" == " "

EM will treat this as is in PHP, "0" == " ", which is false, but in JS LEMval("0") will return 0, not "0", because onlynum = 1, and 0 == " " is true in JS.

DenisChenu

DenisChenu

2017-08-24 13:00

developer   ~44348

-1 < 'A or 1 < 'A' or 1 <= 'A' or … -1 <= '!' => always false. It's the reason of " "arguments type mismatch" part … because PHP cast as string by default …

excet with space … (and empty : empty is 0). Seems javascript set " " to "" then to 0 …

ollehar

ollehar

2017-08-24 13:04

administrator   ~44349

Sure. I was talking about the logic in LEMval(), which will cast "0" to 0, but this doesn't happen in PHP. Basically, I think EM needs to know about the onlynum property, too.

ollehar

ollehar

2017-08-24 13:21

administrator   ~44350

Another idea: Scrap <, >, <= etc and replace them with LEMcompare(operation, type, arg1, arg2) to get precise control over type-casting.

DenisChenu

DenisChenu

2017-08-24 14:18

developer   ~44351

0 == " " : yes seems when using " " , javascript just trim it

Maybe we must treat like '' and '0' , see https://github.com/LimeSurvey/LimeSurvey/blob/master/application/helpers/expressions/em_core_helper.php#L370

Then this fix THIS issue ;)

ollehar

ollehar

2017-08-24 14:19

administrator   ~44352

Yes, I will try that, might fix it for ==.

DenisChenu

DenisChenu

2017-08-24 14:19

developer   ~44353

Something like :

$bNumericArg2 = !trim($arg2[0]) || strval(floatval($arg2[0]))==strval($arg2[0]); https://github.com/LimeSurvey/LimeSurvey/blob/c21c16f990894859626efe28f0749f727b7a4a90/application/helpers/expressions/em_core_helper.php#L287

But need test with " 0 " in js.

DenisChenu

DenisChenu

2017-08-24 14:25

developer   ~44354

Someone already use === in expression ?

ollehar

ollehar

2017-08-24 14:26

administrator   ~44355

My tests run JS too, just tell me which expression to add.

DenisChenu

DenisChenu

2017-08-24 14:52

developer   ~44356

Last edited: 2017-08-24 14:53

View 2 revisions

Maybe :
array('0',1,'WORD')
array('0',1,'NUMBER')
array('0',1,'STRING')

'0' : replace by [space] , '!' (before number in ascii), 'A' (after number in ascii), '0xA' (numeric in PHP not in js), '001.100' (DB and 'maybe' lemval under condition).

comparaison
'!' >= 1
'A' <= 1
' ' <= 1
' ' <= 1
'-1' <= 1
'001.100' + 1 (can give 2 or '001.1001', depend of STRING or not
'001.100' - 1 (0 or NAN ?)

A lot can be added …

'0xA' with 17 and 15 : but i think there are an issue currently

ollehar

ollehar

2017-08-28 15:55

administrator   ~44372

Does this issue only appear for numerical questions?

ollehar

ollehar

2017-08-28 16:31

administrator   ~44373

The answer is no, I can reproduce it with short text question, too.



limesurvey_survey_248764.lss (13,453 bytes)
ollehar

ollehar

2017-08-28 16:50

administrator   ~44374

Last edited: 2017-08-28 16:50

View 2 revisions

I think this is not possible to solve without knowing the question type in EM core, if it's numerical or not, since LEMval() behaves different in that case. So how to add that context in EM core?

DenisChenu

DenisChenu

2017-08-28 16:57

developer   ~44375

EM core : WORD or NUMBER if i don't make error …

DenisChenu

DenisChenu

2017-08-28 16:58

developer   ~44376

NUMBER == isnumeric in js.

DenisChenu

DenisChenu

2017-08-28 17:02

developer   ~44377

BUT : LEMval do : "1" to 1 with any question type not only for numeric question type.

ollehar

ollehar

2017-08-28 17:02

administrator   ~44378

I get all WORD, even for numerical type.

DenisChenu

DenisChenu

2019-06-06 09:03

developer   ~52312

Last edited: 2019-06-06 09:03

View 2 revisions

@ollehar : i work on string/numeric comparaison currently https://bugs.limesurvey.org/view.php?id=14337#c52311

Seems PHP need something like this:

            case '&lt;':
            case 'lt':
                if ($bMismatchType) {
                    $result = array(false, $token[1], 'NUMBER');
                 } elseif ($bBothString && !$bBothNumeric) {
                    $result = array(strcmp($arg1[0],$arg2[0]) &lt; 0 , $token[1], 'NUMBER');
                } else {
                    $result = array(($arg1[0] &lt; $arg2[0]), $token[1], 'NUMBER');
                }
                break;

Maybe it can fix this part too.

ollehar

ollehar

2019-06-06 10:26

administrator   ~52315

Phew... I don't really have the head space to dig into this, but I can help you run the EM tests I created for this bug. It's marked as skipped right now. You need to install NodeJS to run JavaScript without the browser.

DenisChenu

DenisChenu

2019-06-13 11:07

developer   ~52383

PS : i do a pull request but only next week.

  1. Adding strcmp($arg1[0],$arg2[0]) when the 2 are string
  2. Adding a «forced string » check in https://github.com/LimeSurvey/LimeSurvey/blob/7db9677aa40de2c6acc149e41917bd2d29aa705c/application/helpers/expressions/em_core_helper.php#L285 (issue with "3A" < 5+"")
ollehar

ollehar

2019-06-13 11:16

administrator   ~52384

I'd recommend to NOT change anything before having a proper test suite. The chances of breaking something else is very big. :|

DenisChenu

DenisChenu

2019-06-13 11:36

developer   ~52386

Yes, i know it's the reason why i only fix '5' < '20' in https://bugs.limesurvey.org/view.php?id=14337

DenisChenu

DenisChenu

2019-06-14 08:50

developer   ~52410

A precision about type, currently there are, for PHP

  1. WORD : user entered value or answer code : Q00.NAOK. Can be set as number if it can be number. This behaviour was in https://bugs.limesurvey.org/view.php?id=8324. See my point of view here : https://bugs.limesurvey.org/view.php?id=8324#c27049 (and the related issue)
  2. NUMBER : result of a number function (sum) OR Q00.NAOK with a numeric type question (to check)
  3. STRING : result of a string function (join ?)
  4. DQ_STRING and DS_STRING : "a" and 'a'. To mimic JS 2+"" , there are the forced string test

Issue History

Date Modified Username Field Change
2017-08-21 10:47 ollehar New Issue
2017-08-21 10:47 ollehar Status new => assigned
2017-08-21 10:47 ollehar Assigned To => ollehar
2017-08-21 10:47 ollehar File Added: limesurvey_survey_563168.lss
2017-08-21 12:01 ollehar Note Added: 44316
2017-08-21 12:04 ollehar Relationship added related to 07805
2017-08-21 12:24 ollehar Note Added: 44317
2017-08-21 12:27 ollehar Note Added: 44318
2017-08-21 12:27 ollehar Note Added: 44319
2017-08-21 12:32 DenisChenu Note Added: 44320
2017-08-21 12:33 ollehar Note Added: 44321
2017-08-21 12:38 DenisChenu Note Added: 44322
2017-08-21 12:39 ollehar Note Added: 44323
2017-08-21 12:39 ollehar Note Added: 44324
2017-08-21 12:42 DenisChenu Note Added: 44325
2017-08-21 12:44 ollehar Note Added: 44326
2017-08-21 12:53 DenisChenu File Added: limesurvey_survey_emptyMoreTest.lss
2017-08-21 12:53 DenisChenu Note Added: 44327
2017-08-21 13:05 DenisChenu Note Added: 44328
2017-08-21 13:16 ollehar Note Added: 44329
2017-08-21 14:10 ollehar Note Added: 44330
2017-08-21 14:12 ollehar File Added: Selection_263.png
2017-08-21 14:12 ollehar File Added: Selection_262.png
2017-08-21 14:49 DenisChenu Note Added: 44331
2017-08-21 15:06 DenisChenu File Added: limesurvey_survey_emptyMoreMoreTest.lss
2017-08-21 15:06 DenisChenu Note Added: 44332
2017-08-24 11:19 ollehar Note Added: 44338
2017-08-24 11:20 ollehar Note Edited: 44338 View Revisions
2017-08-24 11:22 ollehar Note Edited: 44338 View Revisions
2017-08-24 11:22 ollehar Note Edited: 44338 View Revisions
2017-08-24 11:32 DenisChenu Note Added: 44339
2017-08-24 11:33 ollehar Note Added: 44340
2017-08-24 11:44 DenisChenu Note Added: 44341
2017-08-24 11:46 ollehar Note Added: 44342
2017-08-24 11:50 DenisChenu Note Added: 44343
2017-08-24 11:54 ollehar Note Added: 44344
2017-08-24 11:56 ollehar Note Added: 44345
2017-08-24 12:01 ollehar Note Added: 44346
2017-08-24 12:57 ollehar Note Added: 44347
2017-08-24 13:00 DenisChenu Note Added: 44348
2017-08-24 13:04 ollehar Note Added: 44349
2017-08-24 13:21 ollehar Note Added: 44350
2017-08-24 14:18 DenisChenu Note Added: 44351
2017-08-24 14:19 ollehar Note Added: 44352
2017-08-24 14:19 DenisChenu Note Added: 44353
2017-08-24 14:25 DenisChenu Note Added: 44354
2017-08-24 14:26 ollehar Note Added: 44355
2017-08-24 14:52 DenisChenu Note Added: 44356
2017-08-24 14:53 DenisChenu Note Edited: 44356 View Revisions
2017-08-28 15:55 ollehar Note Added: 44372
2017-08-28 16:31 ollehar File Added: limesurvey_survey_248764.lss
2017-08-28 16:31 ollehar Note Added: 44373
2017-08-28 16:50 ollehar Note Added: 44374
2017-08-28 16:50 ollehar Note Edited: 44374 View Revisions
2017-08-28 16:57 DenisChenu Note Added: 44375
2017-08-28 16:58 DenisChenu Note Added: 44376
2017-08-28 17:02 DenisChenu Note Added: 44377
2017-08-28 17:02 ollehar Note Added: 44378
2019-06-06 09:03 DenisChenu Note Added: 52312
2019-06-06 09:03 DenisChenu Note Edited: 52312 View Revisions
2019-06-06 10:26 ollehar Note Added: 52315
2019-06-13 10:49 ollehar Description Updated View Revisions
2019-06-13 10:50 ollehar Description Updated View Revisions
2019-06-13 11:07 DenisChenu Note Added: 52383
2019-06-13 11:16 ollehar Note Added: 52384
2019-06-13 11:36 DenisChenu Note Added: 52386
2019-06-14 08:50 DenisChenu Note Added: 52410