Yup all the unit tests now fail again.... So the question is, do we have 63 useless incorrect unit tests currently, or is the proposed change wrong... more coffee and more input from others recommended.

@PhilETaylor : My apologies for taking so long to get back to this. For some reason I didn't get (or more likely see) the update from GitHub when it first went through, and only recently went back to check when on the issue/ticket I noticed it still happening.

Also: I think I see what's going on with the failed tests. I believe PHP's regular expressions for floating-point values expect to be matched up against an entire token, but the Joomla tests aren't using a start and end on the RegEx. I can look into the way the PHP parser works, but could someone who's more familiar with this particular project check to verify that the PHPUnit implementation is applying the RegEx without beginning/end of line anchors?

Information is somewhat difficult to come by, but this can be tested using the PHP Tokenizer. A bit more insight is provided by checking the PHP Source Code for the Zend Language Scanner, which shows that the DNUM and EXPONENT_DNUM token types are meant to be processed after already having been separated.

Just quoting @mbabker

My opinion. If PHP can't natively convert .25 to 0.25 I would be careful with adding support for that to our filtering library. As it's supposed to be a filter, I wouldn't add too much magic to make it work as more.

That said, if it does handle it fine, feel free to submit a PR with appropriate unit test coverage (just adding another case to the existing array of cases).

As security sensitive code, I push a little more for changes in JFilterInput to be backed with unit tests than I do for other classes.

. I just wanted to make sure we aren't adding more magic behavior is all.

and quoting you @JTBlum

I’m afraid I’m not experienced enough with Github or with the Joomla core itself to make the change and be at all confident I haven’t broken something else. I was hoping someone else would address the issue.

As the change breaks 63 unit tests, either we are making a very bad change, or we have very bad unit tests for this method.

If its the latter then great! we just need to fix 63 Unit tests to get this to pass the checks... Its not just a case of "adding another case to the existing array of cases", but manually fixing the existing 63 tests and maybe then adding some more...

And I think this might also be important:

Based on what I looked at regarding the different between how the two parsing/cleaning methods are applied, I'm inclined to believe that the unit tests are incorrect because of a mistaken assumption. That said: my point from the previous ticket (quoted above by @PhilETaylor) is still valid. I'm a user of the system that happens to know the language it's written in, not one of the people who maintain it. Asking the experts to take a look at what I found and decide if it had merit was the entire point.

Edit: Not the entire point. I also want the system to improve, but without breaking things in the process.

Run it by @joomla/security

I don't really care what the end result here is and to be honest my opinion is usually the opposite of what people end up doing anyway. Plus, a lot of people seem to think I FUBAR'd the filtering API, so I'm not touching it anymore.

@PhilETaylor your changes don't allow abc-12.456abc to be parsed as float -12.456 I have no idea why our filter accept this as float but changing this is a b/c break.

"your changes" not really "my changes" :-) I just submitted the PR for discussion because @JTBlum brought it up again. See History: #15736 #14156

Im not a member of the @joomla/security JSST and I have no vested interest in this PR...

It seems the right change to make, but the decision, as always, is above my pay grade in this project.

Sorry didn't read the history and didn't remembered it. Just checked the reason for the failed test and they are "valid" from one point of view. The only thing I can tell is that this pr will not go in to j3 with breaking tests.

I was hoping for someone to make the decision before I waded into rewriting 63 tests ;-)

I was hoping for someone to make the decision before I waded into rewriting 63 tests ;-)

The test shouldn't be changed for j3 so the regex have to be fixed and tests extended^^ I hope that's enough decision ;-)

But beside that I'm not sure that accepting .25 as float, on the other side we accept gv3w24g345gh+0.14234vg35b as float...

@SniperSister this needs JSST input please

So once again - this is going no where :)

The test shouldn't be changed for j3

:-)

so the regex have to be fixed and tests extended

It was "fixing" the regex that broke the tests,

Extending the tests is not going to work. Its either/or. Both cannot happen.

This is going nowhere because the proposed fix is wrong.

Alright. I tried

instead of playing with $pattern can try to do something like:

if (substr($source, 0, 1) === '.')
{
	$source = '0' . $source;
}

or maybe not ?

@Fedik I left a pretty detailed comment on #29779 that goes into the regular expression a bit more and outlines how I think people are working with different assumptions. That said: you're right that simpler is often better. PHP's floatval method would be way easier than what's happening, and put the responsibility for sorting out assumptions fall on the PHP team rather than Joomla devs.

Edit: Or I could continue using my own checks, rather than relying on those that don't meet my expectations. I think that may have been what you were driving at.