? Success

User tests: Successful: Unsuccessful:

avatar csthomas
csthomas
19 Feb 2016

Speed up $input->remove() function, replace deprecated stuff.

avatar csthomas csthomas - open - 19 Feb 2016
avatar csthomas csthomas - change - 19 Feb 2016
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 19 Feb 2016
Labels Added: ?
avatar brianteeman brianteeman - change - 19 Feb 2016
Category Tags
avatar Hackwar
Hackwar - comment - 19 Feb 2016

Thank you for looking into this code. I personally have the feeling that JFilterInput is a weakness in Joomla, because it is EXTREMELY complex and I haven't been able to review it, when I looked into this a few years ago because of its complexity. I fear that there is a special string waiting somewhere out there, which is not properly handled by this class. Anyway, back to this PR :wink:

You are right that the code is executed more often than necessary. Can you move the variable assignment outside of the while condition? You could do a do {} while() instead, to run it at least once before checking the result.

It would be cool if we could get unittests for this class. Proper ones, that is.

avatar csthomas
csthomas - comment - 19 Feb 2016

I'm not familiar with the tests, but I like to optimize the code.

avatar csthomas
csthomas - comment - 27 Feb 2016

The information about build "The Travis CI build failed" is incorrect.

Please check status of build at #9163


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

avatar beat
beat - comment - 29 Feb 2016

From a code-review point of view :+1: : The new code is equivalent to the old one and avoids a lot of unneeded cycles (speed optimization).

avatar roland-d roland-d - change - 1 Mar 2016
Milestone Added:
avatar roland-d roland-d - change - 1 Mar 2016
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2016-03-01 08:51:24
Closed_By roland-d
avatar roland-d roland-d - close - 1 Mar 2016
avatar roland-d roland-d - reference | c99806a - 1 Mar 16
avatar roland-d roland-d - merge - 1 Mar 2016
avatar roland-d roland-d - close - 1 Mar 2016
avatar roland-d
roland-d - comment - 1 Mar 2016

I have merged this based on both code reviews and testing the PR myself.

avatar photodude
photodude - comment - 1 Mar 2016

Part of this change is a B/C break,
$source = $this->cleanTags($source); needs to remain as $source = $this->_cleanTags($source);
the underscore-methods need to remain in functions until the underscore-method is depreceated (4.0 in this case) @wilsonge fixed that B\C issue with 6e24ab4

@csthomas would you also consider making make this change at the Framework filter package?

avatar beat
beat - comment - 1 Mar 2016

Woops, missed the underscore removal from visual inspection on github, sorry! :cry:
Thanks @wilsonge for fixing

avatar csthomas
csthomas - comment - 1 Mar 2016

@photodude I will push request on Framework filter package (without underscores)

avatar photodude
photodude - comment - 1 Mar 2016

Yep, the Framework luckily doesn't have the underscore B/C issue to deal with. ;-)
@csthomas Thanks for carrying these changes over to the Framework.

Add a Comment

Login with GitHub to post a comment