Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Fix two places using null instead of array() #1576

Merged
merged 2 commits into from

3 participants

@elinw

No description provided.

@elinw

I'm reasonably sure that the backward compatibility break introduced by adding type hinting was not intentional since there was no discussion about it.

In the absence of unit tests for both the original commit and the change I have tested this patch in a platform application that uses this class.
See also http://joomlacode.org/gf/project/joomla/tracker/?action=TrackerItemEdit&tracker_item_id=29074

@pasamio

It looks like it's expecting an array, what is being passed to it that isn't an array?

@elinw

null

Catchable fatal error: Argument 1 passed to JClientFtp::__construct() must be
an array, null given, called in
/home/bradm/public_html/github2/libraries/joomla/client/ftp.php on line 199 and
defined in /home/bradm/public_html/github2/libraries/joomla/client/ftp.php on
line 139

@pasamio

Why not change getInstance to default to an empty array? Doesn't appear to have a need to be a null.

@elinw

Tried that, same result.

Did you have another thought on this? On the one hand, I feel that if there is an undocumented b/c break for no reason except code style then it should be put it back the way it was and then fix the issue properly so that the code style change can be implemented without the fatal error. On the other hand if it is easy to fix properly of course let's fix. The issue happens on save of the configuration.php file when the ftp fields run a check to make sure the credentials are valid. This is mainly done in JClientHelper setCredentials although there are a lot of moving parts. One thing that has no impact on the fatal error is whether ftp is enabled (well it's more complex than that but it is not a condition of checking the credentials that I can see) which means that once you try and fail at saving you are stuck with the credentials that triggered the error for the rest of the session.

@pasamio

I think if something is sending it a null then the original code is actually broken and we've just uncovered an incorrect calling. Permitting the incorrect use is the wrong step to make. Fortunately we're using isset which will return false for null objects anyway regardless of if the item doesn't exist or if it does exist and is null (e.g. $myArray = array(null); var_dump(isset($myArray[0])); returns a false). It would however return an error if perhaps an object was passed in or some other type. So I feel that the type hint should be fixed and incorrect calling cases need to be correct.

@elinw

yup yup yup. Okay we were right that the one null should be changed to array() but I missed that the fatal error was slightly different when just one was changed. The file system was also creating an instance using null instead of array().

@LouisLandry

Looks good to me. Thanks!

@LouisLandry LouisLandry merged commit 5b61255 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 8, 2012
  1. @elinw
  2. @LouisLandry @elinw

    Fixing up a line ending issue.

    LouisLandry authored elinw committed
    Fix error
This page is out of date. Refresh to see the latest.
View
2  libraries/joomla/client/ftp.php
@@ -189,7 +189,7 @@ public function __destruct()
*
* @since 12.1
*/
- public static function getInstance($host = '127.0.0.1', $port = '21', array $options = null, $user = null, $pass = null)
+ public static function getInstance($host = '127.0.0.1', $port = '21', array $options = array(), $user = null, $pass = null)
{
$signature = $user . ':' . $pass . '@' . $host . ":" . $port;
View
2  libraries/joomla/filesystem/file.php
@@ -394,7 +394,7 @@ public static function write($file, &$buffer, $use_streams = false)
if ($FTPOptions['enabled'] == 1)
{
// Connect the FTP client
- $ftp = JClientFtp::getInstance($FTPOptions['host'], $FTPOptions['port'], null, $FTPOptions['user'], $FTPOptions['pass']);
+ $ftp = JClientFtp::getInstance($FTPOptions['host'], $FTPOptions['port'], array(), $FTPOptions['user'], $FTPOptions['pass']);
// Translate path for the FTP account and use FTP write buffer to file
$file = JPath::clean(str_replace(JPATH_ROOT, $FTPOptions['root'], $file), '/');
Something went wrong with that request. Please try again.