? ? Pending

User tests: Successful: Unsuccessful:

avatar jo-sf
jo-sf
24 Jan 2017

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".

Testing Instructions

Status Quo

  • Open the global configuration form and activate the "System" tab
  • Locate the "Cache Time" field and check its value (the value is "15" by default)
  • Overwrite the default value with "1.5e+1" and press the "Save" button
  • The value in the "Cache Time" field is "1" but it should be "15"
  • Overwrite the value with "15" and press the "Save & Close" button

Change

  • Now install this PR
  • Open the global configuration form and activate the "System" tab
  • Locate the "Cache Time" field and check its value (the value is "15" by default)
  • Overwrite the default value with "1.5e+1" and press the "Save" button
  • The value in the "Cache Time" field is "15"
  • Press the "Cancel" button

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):

  • "0.15e+2" instead of "15"
  • "1.2e+2" instead of "120"
  • "0.12e+3" instead of "120"
  • "1500e-2" instead of "15"
avatar jo-sf jo-sf - open - 24 Jan 2017
avatar jo-sf jo-sf - change - 24 Jan 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Jan 2017
Category Libraries
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

Changed without PR installed Cache Time-Value from 15 to 1.5e+1, after Save there ist 15 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

avatar laoneo
laoneo - comment - 24 Jan 2017

What is the use case for such a change? Just want to know where this will be used.

avatar jo-sf
jo-sf - comment - 24 Jan 2017

@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.

avatar mbabker
mbabker - comment - 24 Jan 2017

This change basically makes the integer case the same as the float case. Somehow, this doesn't seem correct...

avatar jo-sf
jo-sf - comment - 24 Jan 2017

@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:

  • Open the global configuration form and activate the "System" tab
  • Locate the "Cache Time" field and check its value (the value is "15" by default)
  • Overwrite the default value with "1.5e+1" and press the "Save & Close" button
  • Reopen the global configuration and check the "Cache Time" field - it's value is "1" but should be "15"
  • You might check also the configuration.php file, the value for "$cachetime" is "1"
  • Overwrite the value with "15" and press the "Save & Close" button

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.

avatar jo-sf
jo-sf - comment - 24 Jan 2017

@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.

avatar mbabker
mbabker - comment - 24 Jan 2017

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.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

@jo-sf Same Result using "Save" or "Save & Close": 15 is saved after put 1.5e+1 as Value in Cache Time, got also public $cachetime = '15'; in config.php. No Difference if PR or not installed or System Cache is on-conservative, on-progressive or off.

avatar jo-sf
jo-sf - comment - 24 Jan 2017

@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".

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

@jo-sf the difference between #11491 and this pr is surprising me too cause similar instructions.

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

avatar jo-sf jo-sf - change - 24 Jan 2017
Labels Added: ?
avatar joomla-cms-bot joomla-cms-bot - change - 24 Jan 2017
Category Libraries Libraries Unit Tests
avatar jo-sf jo-sf - change - 24 Jan 2017
Labels Added: ?
avatar jo-sf
jo-sf - comment - 24 Jan 2017

@mbabker I just added some additional test cases for JFilterInput::clean() covering the exponential representation with different examples both for integer and unsigned integer cleanup. I hope they work as expected.

avatar jo-sf
jo-sf - comment - 24 Jan 2017

@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.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

@jo-sf #11491 works as describes:

  • Global configuration > "System" tab
  • "Cache Time"-value 15
  • Overwrite with 1.5e+1 > "Save"
  • configuration.php: $cachetime is 1
  • global configuration is 15
  • "Save & Close" button > configuration.php-value for $cachetime is 15

This works as in last successfully Tests.

Network Traffic (cmd+alt+q) don't find where to look (js? xhr? websockets?).

avatar jo-sf
jo-sf - comment - 24 Jan 2017

@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:

  • Open the global configuration and locate the "Cache Time" field
  • Replace the default value "15" with "1.5e+1" and press the "Save & Close" button
  • Check configuration.php: $cachetime is "1" (at least it should be)
  • Reopen the global configuration, the "Cache Time" field probably contains the value "15" but that doesn't really matter - the problem is that configuration.php contains another value
  • Press "Save & Close" an check configuration.php ($cachetime should now be "15")

And after applying the PR:

  • Open the global configuration and locate the "Cache Time" field
  • Replace the default value "15" with "1.5e+1" and press the "Save & Close" button
  • Check configuration.php: $cachetime is "15" (at least it should be)
  • Reopen the global configuration, the "Cache Time" field contains the value "15"
  • Press "Cancel"

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...

avatar mbabker
mbabker - comment - 24 Jan 2017

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).

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 24 Jan 2017

@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.

avatar jo-sf
jo-sf - comment - 25 Jan 2017

@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...

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 25 Jan 2017

I have tested this item successfully on 55899f7

Also #11491 works as described in Test Instructions.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13731.

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 25 Jan 2017 - Tested successfully
avatar jo-sf
jo-sf - comment - 25 Jan 2017

@franz-wohlkoenig I'm very glad to read this - it really seems that yesterday's nightly build didn't contain #11491 ...


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13731.

avatar jo-sf
jo-sf - comment - 25 Jan 2017

@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).


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13731.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 22 Feb 2017

PR needs one more successfully Test.

avatar brianteeman
brianteeman - comment - 4 Jan 2018

I really don't see the need for this.
@mbabker please make a decision

avatar mbabker
mbabker - comment - 4 Jan 2018

As I said before, use the float case as needed.

avatar mbabker mbabker - change - 4 Jan 2018
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2018-01-04 18:12:47
Closed_By mbabker
avatar mbabker mbabker - close - 4 Jan 2018

Add a Comment

Login with GitHub to post a comment