Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[READY] Working on "5" lt "20" #1335

Closed
wants to merge 2 commits into from

Conversation

@Shnoulle
Copy link
Collaborator

Shnoulle commented Oct 22, 2019

Fixed issue : more number VS string comparaison difference in js and php
Dev: add test for "5" le "18"

Dev: my opinion : must have same result in JS and PHP here.
@Shnoulle Shnoulle requested a review from olleharstedt Oct 22, 2019
@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Oct 22, 2019

It's just the test to do here : with Q0 as "5" test done is
https://github.com/LimeSurvey/LimeSurvey/pull/1335/files#diff-5a3f04a9357d7c5320bbb989015c7f62R102-R108

Q00 lt "20" 
Q00+"" lt "20"
Q00+"" lt 20
Q00 ge "20"
Q00+"" ge "20"
Q00+"" ge 20

And the test check only if js result == php result

Dev: still issue with " " and "" string (false string)
@Shnoulle Shnoulle changed the title [WIP] Working on "5" lt "20" [READY] Working on "5" lt "20" Oct 23, 2019
@@ -325,7 +325,7 @@ public function RDP_EvaluateBinary(array $token)
// Not sure if needed to test if [2] is set. : TODO review
if ($bBothNumeric) {
$aForceStringArray = array('DQ_STRING', 'DS_STRING', 'STRING'); // Question can return NUMBER or WORD : DQ and DS is string entered by user, STRING is a result of a String function
if ((isset($arg1[2]) && in_array($arg1[2], $aForceStringArray) || (isset($arg2[2]) && in_array($arg2[2], $aForceStringArray)))) {
if ((isset($arg1[2]) && in_array($arg1[2], $aForceStringArray) && (isset($arg2[2]) && in_array($arg2[2], $aForceStringArray)))) {

This comment has been minimized.

Copy link
@olleharstedt

olleharstedt Oct 23, 2019

Contributor

For me, there's absolutely no way to know if this change breaks anything without a complete test suite. :|

This comment has been minimized.

Copy link
@Shnoulle

Shnoulle Oct 23, 2019

Author Collaborator

The issue is (in 3.19) : JS and PHP does 2 different think.

See the 1st commit : 84e7ebe
I test ONLY if js value == php value

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Oct 23, 2019

I'll checkout this PR and have a look.

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Oct 23, 2019

Shouldn't + be banned on strings too? Use strconcat or something.

Banned = emit warning (not error).

@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Oct 23, 2019

About + maybe warning always … use sum or join

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Oct 23, 2019

+ on numbers is OK because it's easy to have same behvaiour on JS and PHP. IMHO.

@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Oct 23, 2019

Yes, but 2 + "" + 2 => number or string ? ("" come from a no answer value)

It's not a JS vs PHP issue, it's desired behaviour issue

@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Oct 23, 2019

And 2 + QCODE_SQ01.NAOK + "2" with QCODE_SQ01 filtered ? number or string

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Oct 23, 2019

2 + "" is + used on string, so should be warning.

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Oct 23, 2019

So + only on questions that are forced to numerical. They still have empty string as no answer?

@olleharstedt

This comment has been minimized.

Copy link
Contributor

olleharstedt commented Oct 23, 2019

Otherwise, to use + you must first intval(NAOK).

@Shnoulle

This comment has been minimized.

Copy link
Collaborator Author

Shnoulle commented Nov 13, 2019

@olleharstedt : now there are warnings : did i merge and test this one again ?

@Shnoulle Shnoulle requested a review from olleharstedt Nov 13, 2019
@Shnoulle Shnoulle closed this Nov 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.