? Pending

User tests: Successful: Unsuccessful:

avatar zero-24
zero-24
27 Mar 2020

Summary of Changes

Make sure the Itemid filter always casts to integer, when the itemid get passed an array of items the current getint filter (int, integer) returns an array but the itemid should never be an array.

Testing Instructions

Make sure the usual Frontend and backend still works and the correct itemid is in the html classes.

Expected result

We type cast the itemid to integer

Actual result

Passing an array would result into the itemId not to be an actual ID any more but an array.

Documentation Changes Required

None cc @SniperSister @HLeithner

Reported by: https://twitter.com/paulosyibelo

avatar zero-24 zero-24 - open - 27 Mar 2020
avatar zero-24 zero-24 - change - 27 Mar 2020
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 27 Mar 2020
Category Administration Templates (admin) Front End com_contact com_content com_search com_tags Layout External Library Libraries Modules Templates (site)
avatar zero-24 zero-24 - change - 27 Mar 2020
Labels Added: ?
avatar zero-24 zero-24 - change - 27 Mar 2020
The description was changed
avatar zero-24 zero-24 - edited - 27 Mar 2020
avatar laoneo
laoneo - comment - 2 Apr 2020

I'm using on tons of places getItn since ages, if this is not safe anymore and doesn't always return an integer then we need to inform the extension devs about this change.

avatar laoneo
laoneo - comment - 3 Apr 2020

Is there a real situation why we need this change? Would be good to have a test case in core to reproduce the issue.
For me it looks strange when we have a function getInt and then have to do an int cast, especially on Itemid which is never an array. It is even more awkward that an array will be casted to integer. Because casting an array to integer doesn't return the value, it returns always 1, so it would be wrong anyway.
Execute var_dump((int)[2]);and it will return 1. So if you want to handle that Itemid can be an array then do it right and make a type check and work with arrays properly.

avatar SharkyKZ
SharkyKZ - comment - 3 Apr 2020

Is there a real situation why we need this change?

See #26753. This is needed to avoid errors when Itemid is being manipulated by user. You're right, though, this should not be cast like that. Should default to 0 or whatever we default to now.

Also test failure is related.

avatar SharkyKZ
SharkyKZ - comment - 3 Apr 2020

@zero-24 Are changes in FoF files necessary? If so, they should include a note that files have been modified.

avatar laoneo
laoneo - comment - 3 Apr 2020

But would it then not be better to ensure that Itemid is always an integer before core gets executed instead afterwards?

avatar mbabker
mbabker - comment - 22 Apr 2020

I'm using on tons of places getItn since ages, if this is not safe anymore and doesn't always return an integer then we need to inform the extension devs about this change.

It's still safe to use. The array support traces back to joomla-framework/filter#13.

Basically since early 2016, stable releases of the joomla/filter package have supported cleaning either a single item or an array of items using a given filter. So InputFilter::getInstance()->clean(['1', 2, '3', 4], 'int'); should be returning a cleaned array of integers. Without that you get array to string conversion errors when working with request data that you aren't expecting to be in an array, and there isn't a simple way to deal with that without breaking the inbuilt filtering from the joomla/input API on any get() call.

But would it then not be better to ensure that Itemid is always an integer before core gets executed instead afterwards?

I would suggest casting here is the wrong behavior. The Itemid input variable probably needs to be checked early and the application just flat out abort with some kind of Exception if the Itemid input variable is an array; an array is flat out invalid data and should be rejected and casting is going to accept it and create potentially unwelcome side effects. Same goes for other core input variables (i.e. view, format, option, tmpl, layout, task for the "not so legacy legacy" MVC, type for the feed views, all of which are the main system variables; at the component level you've got input variables like id that should also be checked for array versus single item). Basically a lack of proper input validation is letting some fields which are expected to be single items come in as arrays which causes problems (BTW the problem isn't the array support in the filter class, it's the fact that the data is coming out of the request superglobals as an array then going into the filter API before there is a chance to even check the data structure is valid).

avatar laoneo
laoneo - comment - 22 Apr 2020

Made an alternative #28751, where the Itemid is correctly set when it is an array.

avatar zero-24
zero-24 - comment - 22 Apr 2020

Awesome thanks @laoneo closing here ?

avatar zero-24 zero-24 - change - 22 Apr 2020
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2020-04-22 11:04:57
Closed_By zero-24
avatar zero-24 zero-24 - close - 22 Apr 2020

Add a Comment

Login with GitHub to post a comment