-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Conversation
pomaxa
commented
Oct 3, 2015
- shorten namespaces
- fix calling static methods as non static
- joins - implode change
* fix calling static methods as non static * joins - implode change
@@ -263,7 +263,7 @@ public static function shuffle($arg = '') | |||
* @param array $array The set to shuffle | |||
* @return array The shuffled set | |||
*/ | |||
public static function shuffleArray($array = array()) | |||
public static function shuffleArray(array $array = array()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not at all.
- in php doc we got: "@param array $array The set to shuffle"
- in unit tests there is no check for behavior of shuffleArray in case of passing non array there.
- all usage of shuffleArray -> always pass array into
- in case of someone tries to pass non array,
PHP Warning: reset() expects parameter 1 to be array, string given in php shell code on line 1
will be thrown, - cuz reset()
is calling on $array var without checking it.
@GrahamCampbell - please point me on where is BC, - cuz I do not see any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the signature of a public or protected function on a non-final class is breaking for anyone extending the class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GrahamCampbell , but this method willcall en php error in case non array passed, + param type is written in phpdoc ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this change, if someone can explain why it is an BC; I understand that it's looks like it, but I don't see any situation, where it will be a problem. Cuz this function can't be used without an array as param.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You would break this:
class CustomBase extends Base {
public static function shuffleArray($array = array()) {
// ...
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
class Base2 extends Faker\Provider\Base
{
public static function shuffleArray($array = array()) {
return $array;
}
}
$generator = new \Faker\Generator();
$b = new Base2($generator);
var_dump($b->shuffleArray(array('a','b')));
and there is no error; what the problem ?
php -v 1:37 pmx@pmx-K56CB
PHP 5.6.4-4ubuntu6.2 (cli) (built: Jul 2 2015 15:29:28)
Copyright (c) 1997-2014 The PHP Group
Zend Engine v2.6.0, Copyright (c) 1998-2014 Zend Technologies
with Zend OPcache v7.0.4-dev, Copyright (c) 1999-2014, by Zend Technologies
@vlakoff , @GrahamCampbell - I will be really thankful, if you guys can explain what will be BC, what this will break, and how it can affect users?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very simply, there will be a fatal error...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ish. This is what actually happens: https://3v4l.org/m3RLg.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GrahamCampbell thanks. I've forgot to enable errors; It really throw warning for this case. I have removed this changes.
Thanks! |