User tests: Successful: Unsuccessful:
Allow the exponential representation for integers and unsigned integers too since modern browsers accept e.g. "1.2e+1" as a number with the value "12".
Status Quo
Change
You might test this with other numeric fields (e.g. the "Session Lifetime" field) and other exponential representations too. Some ideas for testing (not all of these exponential representations are correct in the sense of a mathematician but they all should work when this PR is installed):
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
What is the use case for such a change? Just want to know where this will be used.
@laoneo An administrator might try to enter numeric values in this exponential representation, especially if the numbers are greater this might ease the input. Beside that @zero-24 suggested in #11491 that I might create a PR that covers this and converts the exponential representation into the standard numeric form.
This change basically makes the integer case the same as the float case. Somehow, this doesn't seem correct...
@franz-wohlkoenig I tested this with the most current staging version identifying itself as "Joomla! 3.7.0-beta1" (I installed this version this morning, and it contains - beside many other PRs I assume - the #11491 PR which might make the difference here).
So can you please retry without PR installed as follows:
The test instructions with this PR installed need to be adapted such that you press "Save & Close" instead of "Save" only and reopen the global configuration afterwards.
If either PR #11491 is installed before starting the tests or if the Joomla! version used for testing is recent enough that it already contains the #11491 change (this PR has been merged yesterday) you can use the test instructions as described in my initial comment.
@mbabker Well, not exactly. The search pattern is identical for the integer and the float cases (this seems to be clear since "1.5e+1" is in floating point format and the floating point search pattern also matches integers) but the conversion of the match found makes the difference since the two casts "(int) (float)
" guarantee that the match is first converted into a float and then converted into an integer (a direct cast to integer doesn't work). And finally the result will be an integer and not a float.
The tests applied to numbers (this is done prior to the final cleanup and conversion btw) always use casts to float only - no matter whether the field is defined as float or integer.
You might argue that the pattern also matches e.g. "1.51e+1" which is no integer. This is right but modern browsers mark this input in a numeric field as an error and they won't send it to the server. And even if you manage to send this to the server it will be converted to "15" which is not correct but not as wrong as the conversion to "1" which happens without my PR.
To be honest, my first read on this PR suggests that the downstream user should just use the float case if they need to support exponential representation in their inputs then handle any rounding or anything if they need the resulting value to be a whole number.
Considering this is security sensitive code, I think we need to be careful about adding features like this. The behavior needs to be validated and ensures that all return values are correct (so this needs to add unit test cases at a minimum).
I think I might look at it differently if we didn't already offer a way to support exponential representations but as we already have that support (and IMO in the case it makes the most sense with), I'm not 100% sold on adding it yet to the other cases.
@franz-wohlkoenig Which browser do you use? I tried this with Firefox 50.1.0 and IE 11.0 - both with the same result. And in both cases the value "1.5e+1" is sent to the server (I found the following data in the POST body: jform[cachetime]=1.5e+1
).
Other web browsers might behave differently.
And what annoys me a bit: when you tested #11491 you flagged the test as successful - and this was only three days ago. The test instructions for #11491 and the test instructions for this PR are very similar (which is clear since the intention for this PR came from the #11491 PR). So if you repeat the test instructions from #11491 with your current 3.7.0-beta1-nightly version it should behave as described unter "Change".
Labels |
Added:
?
|
Category | Libraries | ⇒ | Libraries Unit Tests |
Labels |
Added:
?
|
@franz-wohlkoenig Can you please repeat the #11491 tests with your current version? You should get the behaviour as described under "Change" since that PR has been merged into base branch yesterday. If those tests should fail now we have to investigate this in depth, but if they still work (as I hope) we should get this PR and the tests for it also to work.
When performing the #11491 tests you might activate the built-in debugger (F12 under Windows, MacOS???) and inspect the network traffic, especially what is sent to the server if you first entered "1.5e+1" into the "Cache Time" field and the pressed the "Save & Close" button. With all browsers I checked with the browser sends "1.5e+1" to the server, but there might be cases where the browser already converts the user's input and sends the converted input to the server.
@jo-sf #11491 works as describes:
15
1.5e+1
> "Save"$cachetime
is 1
15
$cachetime
is 15
This works as in last successfully Tests.
Network Traffic (cmd+alt+q) don't find where to look (js? xhr? websockets?).
@franz-wohlkoenig Thanks for repeating the #11491 tests, but as I see you got the "Status Quo" results when doing these tests. I assumed that #11491 is also part of 3.7.0-beta1-nightly since it has been merged yesterday (and I got it when downloading the current staging version), but this is not the case...
I cannot say how long it takes until all PRs merged into the staging code are in the nightly builds.
If you look into the network traffic you should see a POST call to the server when you press the "Save" or the "Save & Close" button. The body of this POST call contains all configuration form fields, and beside many others you can find there the field jform[cachetime]
(the "[" and "]" are encoded as "%5B" and "%5D").
Let's try it this way:
$cachetime
is "1" (at least it should be)$cachetime
should now be "15")And after applying the PR:
$cachetime
is "15" (at least it should be)The reason for this behaviour (and the difference between my test description and your test results) might be that Joomla stores the global configuration with the session and reads it again when you next contact your session (next operation). At least this is my assumption since I had to store once more when building #11491. So if you log out after saving the global configuration with "1.5e+1" as the cache time and then log in again (new session) you probably get the value "1" even in the global configuration and not only in the configuration.php file.
We'll get this thing to work...
I cannot say how long it takes until all PRs merged into the staging code are in the nightly builds.
The timestamp when the cron job was triggered is displayed right on the nightly builds page as well as the commit hash each package was built from. The cron runs at 02:00 UTC daily (hence a nightly build, we aren't building packages on each commit).
@jo-sf I'm on "Custom URL: https://update.joomla.org/core/nightlies/next_minor_list.xml" and update every Morning. This should be the latest build.
Will test on tomorrow.
@franz-wohlkoenig I just checked the latest nightly build (Wednesday, 25 January 2017 02:00:17 UTC) whether it contains #11491 - and it does. I cannot say however whether yesterday's nightly build also contained that PR already (it seems as if only the most recent nightly build is available).
So if you repeat the #11491 tests with the latest nightly build you should no longer get the "Status Quo" results but instead the results as described after having applied that PR. And if my assumption is correct you should also get the "Status Quo" results for the current PR regarding exponential representation for numbers - at least I hope so.
My apologies that I was so fast with this PR which might have caused your problems in testing it with the latest nightly build (I'm normally testing with the current staging code which can be newer that the nightly builds). If I would have waited one day longer...
I have tested this item
Also #11491 works as described in Test Instructions.
@franz-wohlkoenig I'm very glad to read this - it really seems that yesterday's nightly build didn't contain #11491 ...
@mbabker Currently you can enter something like "1.5e+1" into a numeric field in any modern browser and the browser accepts this as a valid number. This value is then sent to the server and Joomla currently converts this into "1" which is (I think) not the desired result. This PR is meant to change this behaviour such that an exponential representation is accepted (which is the case even without my PR since all testing is done by casting the value to float) and converted correctly (which is currently not the case). But after applying this PR the input "1.5e+1" will be converted correctly into the value "15".
I found this by chance when I tested #11322 and I used such input for testing #11491, and in the context of the latter PR @zero-24 suggested that I should create another PR covering that.
This PR has nothing to do with replacing float or double fields since those allow to have decimal places which is not the case here (after conversion).
PR needs one more successfully Test.
As I said before, use the float case as needed.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-01-04 18:12:47 |
Closed_By | ⇒ | mbabker |
Changed without PR installed
Cache Time
-Value from15
to1.5e+1
, afterSave
there ist15
as Value.Test on:
Joomla! 3.7.0-beta1-nightly
macOS Sierra, 10.12.2
Firefox 50.1.0
PHP 7.0.4
MySQLi 5.5.53-0