? Pending

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
4 Apr 2021

Code review

The min() function in PHP returns a mixed and not int

https://www.php.net/manual/en/function.min.php

avatar PhilETaylor PhilETaylor - open - 4 Apr 2021
avatar PhilETaylor PhilETaylor - change - 4 Apr 2021
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 4 Apr 2021
Category Libraries
avatar HLeithner
HLeithner - comment - 5 Apr 2021

Not sure about this on, I mean it's a b/c break changing this, so maybe cast to int would be better and possible?

avatar PhilETaylor
PhilETaylor - comment - 5 Apr 2021

well that your call. I don't see how following thePHP documentation is a b.c break though.

avatar ceford ceford - test_item - 5 Apr 2021 - Tested successfully
avatar ceford
ceford - comment - 5 Apr 2021

I have tested this item successfully on 68cedf8

I looked at the code and noted that getMaxUploadSize is used in three places and as this is just a correction to the description of what is returned I think it is OK. If it were to be cast as an int that would be a change that might have b/c consequences. I declare myself a php amateur here so agree it is for the experts to call.


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

avatar PhilETaylor
PhilETaylor - comment - 5 Apr 2021

I looked at the code and noted that getMaxUploadSize is used in three places

In Joomla core. You do not know how many calls this method gets (if any) from 3rd Party code in plugins/extensions (probably none, but that's not the point.

Before this PR this method ALWAYS returned with a type of (theoretically) mixed as that is literally the PHP documented type returned from min() according to https://www.php.net/manual/en/function.min.php

min() returns the parameter value considered "lowest" according to standard comparisons. If multiple values of different types evaluate as equal (e.g. 0 and 'abc') the first provided to the function will be returned.

If an empty array is passed, then false will be returned and an E_WARNING error will be emitted.

Therefore, to now cast the return to an int is (theoretically) a backward incompatible change.

Changing the docblock to factually state that the return type from the min is mixed is fully backward compatible.

avatar ceford
ceford - comment - 5 Apr 2021

@PhilETaylor on your last comment I think that is all what I said or meant. You are working too hard. But keep going!

avatar PhilETaylor
PhilETaylor - comment - 5 Apr 2021

My last part was disagreeing with @HLeithner

avatar HLeithner HLeithner - change - 5 Apr 2021
Status Pending Fixed in Code Base
Closed_Date 0000-00-00 00:00:00 2021-04-05 18:15:10
Closed_By HLeithner
Labels Added: ?
avatar HLeithner HLeithner - close - 5 Apr 2021
avatar HLeithner HLeithner - merge - 5 Apr 2021
avatar HLeithner
HLeithner - comment - 5 Apr 2021

In the end it's like this several years so better documentation follows code then the way around (at least in this case)

avatar PhilETaylor
PhilETaylor - comment - 5 Apr 2021

Don't get me started with how bad some of the docblocks are in joomla 3 haha

Add a Comment

Login with GitHub to post a comment