Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC][DISCUSSION] Strict Argument Count »

[RFC][DISCUSSION] Strict Argument Count

php.internals

Marcio Almada

11 years ago
Hi, internals I'm moving the "Strict Argument Count" RFC into discussion phase: RFC: https://wiki.php.net/rfc/strict_argcount PR: https://github.com/php/php-src/pull/1108 Many different opinions were collected during research phase and the RFC was updated with real BC break measurements and other important sections. So, before discussing: - Even if you already read the RFC in the past, read it again now. - Don't claim **possible** massive BC breaks before read the measurements already done. No matter how seasoned you are with PHP, real numbers matter most than assumptions. Your measurements are welcome too. - Try the patch. Really. - Consider reading the use case present on this post: goo.gl/3ykdIy - Keep discussion on topic and remember we are all trying to improve PHP in some way :) Thanks, Márcio

Yasuo Ohgaki

11 years ago
Hi Marcio, On Mon, Mar 2, 2015 at 8:02 AM, Marcio Almada <marcio.web2@gmail.com> wrote:
> I'm moving the "Strict Argument Count" RFC into discussion phase: > > RFC: https://wiki.php.net/rfc/strict_argcount > PR: https://github.com/php/php-src/pull/1108 > > Many different opinions were collected during research phase and the RFC > was updated with real BC break measurements and other important sections. > So, before discussing: > > - Even if you already read the RFC in the past, read it again now. > - Don't claim **possible** massive BC breaks before read the > measurements already done. No matter how seasoned you are with PHP, real > numbers matter most than assumptions. Your measurements are welcome too. > - Try the patch. Really. > - Consider reading the use case present on this post: goo.gl/3ykdIy > - Keep discussion on topic and remember we are all trying to improve PHP > in some way :) >
I like the idea. /** fn expects a variable-length argument lists */ function fn($arg) { $arg = func_get_arg(); $args = func_get_args(); } fn(1); // Ok fn(...[1, 2, 3, 4, 5]); // Ok call_user_func_array("fn", [1, 2, 3, 4, 5, 6, 7]); // Ok I understand motivation why your patch behave like this. It's for BC, right? However, isn't it better to declare variable length parameters by function signature in the long run? function fn($arg, ...) {} Is it possible to have E_DEPRECATED error without "..."? and do not care about func_get_arg*() existence? Make E_DEPRECATED error E_WARNING in PHP 7.2 or 7.3. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Marcio Almada

11 years ago
Hi Yasuo, 2015-03-02 1:43 GMT-03:00 Yasuo Ohgaki <yohgaki@ohgaki.net>:
> Hi Marcio, > > On Mon, Mar 2, 2015 at 8:02 AM, Marcio Almada <marcio.web2@gmail.com> > wrote: > > I like the idea. > > /** fn expects a variable-length argument lists */ > function fn($arg) { > $arg = func_get_arg(); > $args = func_get_args(); > } > > fn(1); // Ok > fn(...[1, 2, 3, 4, 5]); // Ok > call_user_func_array("fn", [1, 2, 3, 4, 5, 6, 7]); // Ok > > I understand motivation why your patch behave like this. It's for BC, > right? >
Yes. If you search github for func_get_arg or func_get_args you wil get around 2,734,673 results. That's a lot.
> However, isn't it better to declare variable length parameters by function > signature > in the long run? > > function fn($arg, ...) {} > > Is it possible to have E_DEPRECATED error without "..."? and do not care > about func_get_arg*() existence? Make E_DEPRECATED error E_WARNING > in PHP 7.2 or 7.3. > >
I'm not against doing this in a future when PHP v5.5 starts to fade away. But, right now, a lot of code still needs to support PHP 5.5+ with no alternative other than use func_get_args. Remember we only got the dedicated syntax for variadic functions recently on PHP v5.6. I'm afraid it's too soon to deprecate func_get_arg*s() or to overlook it. Maybe in the future somebody will build upon this RFC, specially if it gets approval, and start the deprecation. But at current pace I don't see this as an alternative unless we all reach consensus, which is unlikely. Another point is that internal functions are currently using warnings to signalize wrong argument counts: strlen <http://www.php.net/strlen>("foo", "bar"); // PHP warning: strlen() expects exactly 1 parameter, 2 given on line 1 If the mailing list reach consensus that we should emit deprecation instead, maybe internal functions will need to be updated too. I'm not against it also, but I think it would be too soon for a lot of people here. Regards,
> > -- > Yasuo Ohgaki > yohgaki@ohgaki.net >
Thanks, Márcio

Yasuo Ohgaki

11 years ago
Hi Marcio, On Mon, Mar 2, 2015 at 3:07 PM, Marcio Almada <marcio.web2@gmail.com> wrote:
> 2015-03-02 1:43 GMT-03:00 Yasuo Ohgaki <yohgaki@ohgaki.net>: > >> Hi Marcio, >> >> On Mon, Mar 2, 2015 at 8:02 AM, Marcio Almada <marcio.web2@gmail.com> >> wrote: >> >> I like the idea. >> >> /** fn expects a variable-length argument lists */ >> function fn($arg) { >> $arg = func_get_arg(); >> $args = func_get_args(); >> } >> >> fn(1); // Ok >> fn(...[1, 2, 3, 4, 5]); // Ok >> call_user_func_array("fn", [1, 2, 3, 4, 5, 6, 7]); // Ok >> >> I understand motivation why your patch behave like this. It's for BC, >> right? >> > > Yes. If you search github for func_get_arg or func_get_args you wil get > around 2,734,673 results. That's a lot. > > >> However, isn't it better to declare variable length parameters by >> function signature >> in the long run? >> >> function fn($arg, ...) {} >> >> Is it possible to have E_DEPRECATED error without "..."? and do not care >> about func_get_arg*() existence? Make E_DEPRECATED error E_WARNING >> in PHP 7.2 or 7.3. >> >> > I'm not against doing this in a future when PHP v5.5 starts to fade away. > But, right now, a lot of code still needs to support PHP 5.5+ with no > alternative other than use func_get_args. Remember we only got the > dedicated syntax for variadic functions recently on PHP v5.6. > > I'm afraid it's too soon to deprecate func_get_arg*s() or to overlook it. > Maybe in the future somebody will build upon this RFC, specially if it gets > approval, and start the deprecation. But at current pace I don't see this > as an alternative unless we all reach consensus, which is unlikely. > > Another point is that internal functions are currently using warnings to > signalize wrong argument counts: > > strlen <http://www.php.net/strlen>("foo", "bar"); > // PHP warning: strlen() expects exactly 1 parameter, 2 given on line 1 > > If the mailing list reach consensus that we should emit deprecation > instead, maybe internal functions will need to be updated too. I'm not > against it also, but I think it would be too soon for a lot of people here. >
I understand your reasons. Compatibility is important, but detecting function body contents and suppressing errors by engine is too hacky. Raising E_DEPRECATE/E_STRICT by function definition seems the way to go. IMO. [yohgaki@dev github-php-src]$ php -r 'function f(...$a) {var_dump($a);}; f(1,2,3);' array(3) { [0]=> int(1) [1]=> int(2) [2]=> int(3) } [yohgaki@dev github-php-src]$ php -r 'function f($a) {var_dump($a);}; f(1,2,3);' int(1) is current behavior. The latter would be [yohgaki@dev github-php-src]$ php -r 'function f($a) {var_dump($a);}; f(1,2,3);' Deprecated: Excessive argument for f() without "...", called in Command line code on line 1 and defined in Command line code on line 1 int(1) User may have a lot of E_DEPRECATED errors anyway if strict scalar type hint passes. I like the idea. Please use function definition for errors. (E_DEPRECTED or E_STRICT whichever is suitable) Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Marcio Almada

11 years ago
Hi, 2015-03-03 16:48 GMT-03:00 Yasuo Ohgaki <yohgaki@ohgaki.net>:
> Hi Marcio, > >
I understand your reasons. Compatibility is important, but detecting
> function body contents and > suppressing errors by engine is too hacky. Raising E_DEPRECATE/E_STRICT by > function definition seems > the way to go. IMO. > >
Just one correction, we are not "suppressing" the warning. It just emits the warning conditionally and that's not hacky at all. We already have other situations where warnings are emitted just when certain conditions are met.
> [yohgaki@dev github-php-src]$ php -r 'function f(...$a) {var_dump($a);}; > f(1,2,3);' > array(3) { > [0]=> > int(1) > [1]=> > int(2) > [2]=> > int(3) > } > [yohgaki@dev github-php-src]$ php -r 'function f($a) {var_dump($a);}; > f(1,2,3);' > int(1) > > is current behavior. The latter would be > > [yohgaki@dev github-php-src]$ php -r 'function f($a) {var_dump($a);}; > f(1,2,3);' > Deprecated: Excessive argument for f() without "...", called in Command > line code on line 1 and defined in Command line code on line 1 > int(1) > >
Using only the function signatures, overlooking *func_get_args()*, would be in practice an indirect deprecation of* func_get_args()* and that's not negotiable at all, at least on this RFC. We simply can't do this now because it would be an enormous BC break and PHP 5.5, which has no "first class" variadics, is still supported. I understand the reasons you want to do it, but I think we should wait a bit more before discussing a possible *func_get_args* deprecation (if it will ever be deprecated). That's not the point addresses by this RFC.
> User may have a lot of E_DEPRECATED errors anyway if strict scalar type > hint passes. >
That's highly speculative. We don't even know if the proposal of scalar typehints with BC breaks will ever get voted, neither if it will pass. It's not a good idea to take decisions based upon that. Don't you think so?
> I like the idea. Please use function definition for errors. (E_DEPRECTED > or E_STRICT whichever > is suitable) > >
I'm proposing a warning because currently PHP internal functions already use a warning to indicate wrong argument count. Using E_STRICT or whichever would look inconsistent IMMO at a first sight. But I think that E_STRICT || E_WARNING || E_DEPRECATED is a very debatable topic. Regards,
> > -- > Yasuo Ohgaki > yohgaki@ohgaki.net > >
Márcio.

Yasuo Ohgaki

11 years ago
Hi Marcio, On Thu, Mar 5, 2015 at 4:07 AM, Marcio Almada <marcio.web2@gmail.com> wrote:
> 2015-03-03 16:48 GMT-03:00 Yasuo Ohgaki <yohgaki@ohgaki.net>: > >> Hi Marcio, >> >> > I understand your reasons. Compatibility is important, but detecting >> function body contents and >> suppressing errors by engine is too hacky. Raising E_DEPRECATE/E_STRICT >> by function definition seems >> the way to go. IMO. >> >> > Just one correction, we are not "suppressing" the warning. It just emits > the warning conditionally and that's not hacky at all. We already have > other situations where warnings are emitted just when certain conditions > are met. > > >> [yohgaki@dev github-php-src]$ php -r 'function f(...$a) {var_dump($a);}; >> f(1,2,3);' >> array(3) { >> [0]=> >> int(1) >> [1]=> >> int(2) >> [2]=> >> int(3) >> } >> [yohgaki@dev github-php-src]$ php -r 'function f($a) {var_dump($a);}; >> f(1,2,3);' >> int(1) >> >> is current behavior. The latter would be >> >> [yohgaki@dev github-php-src]$ php -r 'function f($a) {var_dump($a);}; >> f(1,2,3);' >> Deprecated: Excessive argument for f() without "...", called in Command >> line code on line 1 and defined in Command line code on line 1 >> int(1) >> >> > Using only the function signatures, overlooking *func_get_args()*, would > be in practice an indirect deprecation of* func_get_args()* and that's > not negotiable at all, at least on this RFC. > > We simply can't do this now because it would be an enormous BC break and > PHP 5.5, which has no "first class" variadics, is still supported. I > understand the reasons you want to do it, but I think we should wait a bit > more before discussing a possible *func_get_args* deprecation (if it will > ever be deprecated). That's not the point addresses by this RFC. >
I don't think we need to deprecate func_get_args(). We may have <?php function f($a, ...) { var_dump(func_get_args()); } f(1,2,3); ?> array(3) { [0]=> int(1) [1]=> int(2) [2]=> int(3) } "...$a" packs arguments. Using "..." as variable parameter indication may be allowed. Strictly speaking, it's not needed, though.
> > >> User may have a lot of E_DEPRECATED errors anyway if strict scalar type >> hint passes. >> > > That's highly speculative. We don't even know if the proposal of scalar > typehints with BC breaks will ever get voted, neither if it will pass. It's > not a good idea to take decisions based upon that. Don't you think so? >
It's related, but it's different issue.
> > >> I like the idea. Please use function definition for errors. (E_DEPRECTED >> or E_STRICT whichever >> is suitable) >> >> > I'm proposing a warning because currently PHP internal functions already > use a warning to indicate wrong argument count. Using E_STRICT or whichever > would look inconsistent IMMO at a first sight. But I think that E_STRICT || > E_WARNING || E_DEPRECATED is a very debatable topic >
I don't care much about error types, because I change code so that any errors are not raised anyway. E_WARNING in this case makes sense because users made obvious errors. I understand you would like to address errors in older code with compiler change. We don't have to care much for older broken code. IMO. I understand importance of argument count check. Your wordpress research clearly shows that we must raise error for wrong number of arguments. Looking into function body and checking certain function existence seems just too much for a language. However, I don't have too strong opinion for this. I would like to have comments from engine developers. Any thoughts? Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Marcio Almada

11 years ago
Hi, 2015-03-04 21:51 GMT-03:00 Yasuo Ohgaki <yohgaki@ohgaki.net>:
> > I don't think we need to deprecate func_get_args(). We may have > > <?php > function f($a, ...) { > var_dump(func_get_args()); > } > > f(1,2,3); > ?> > > "...$a" packs arguments. Using "..." as variable parameter indication may > be allowed. > Strictly speaking, it's not needed, though. > >
I think you are missing a "detail" here. Even if we add "..." to the language, people on PHP 5.5 would not be able to have it because this new syntax would be available only on PHP 7.0+ . We still have to account for func_get_args otherwise people maintaining packages for both PHP 7 and 5.5 will have no alternative to fix the warning. Do you get me?
> >> >> I like the idea. Please use function definition for errors. (E_DEPRECTED >>> or E_STRICT whichever >>> is suitable) >>> >>> >> I'm proposing a warning because currently PHP internal functions already >> use a warning to indicate wrong argument count. Using E_STRICT or whichever >> would look inconsistent IMMO at a first sight. But I think that E_STRICT || >> E_WARNING || E_DEPRECATED is a very debatable topic >> > > I don't care much about error types, because I change code so that any > errors are not raised anyway. > E_WARNING in this case makes sense because users made obvious errors. > >
Great, so warning seems ok and it's good to keep consistency with other already well established language behaviors.
> I understand you would like to address errors in older code with compiler > change. We don't have to > care much for older broken code. IMO. I understand importance of argument > count check. Your > wordpress research clearly shows that we must raise error for wrong > number of arguments. > >
It's not "old broken code", it's PHP 5.5+ code and I already told you that this would be a bad BC break because: 1. people still have to support PHP 5.5, emit a warning ignoring func_get_args will only cause pain and won't these people. 2. this would impede the RFC to pass because we would now have a major BC break instead of a minor helpful one, and the RFC was proposed to help people, you saw the uses cases on the RFC.
> Looking into function body and checking certain function existence seems > just too much for a > language. However, I don't have too strong opinion for this. I would like > to have comments from engine > developers. Any thoughts? >
It would be nothing new or weird. It's just a simple compile time check. We already "inspect" code in many other situations while compiling and a lot of behaviors during execution already depend on compile time checks. I don't think this is "too much". It might sound unusual, at first, but there is nothing wrong with this.

Yasuo Ohgaki

11 years ago
Hi Marcio, On Fri, Mar 6, 2015 at 6:36 AM, Marcio Almada <marcio.web2@gmail.com> wrote:
> It would be nothing new or weird. It's just a simple compile time check. > We already "inspect" code in many other situations while compiling and a > lot of behaviors during execution already depend on compile time checks. I > don't think this is "too much". It might sound unusual, at first, but there > is nothing wrong with this.
I really like your idea. You proved usefulness very well by your research. My only concern is non-language construct detection by compiler. I'll vote "yes" regardless of my suggestion if it's not an issue. Zeev suggested better path for migration, not to raise errors (E_ERROR/E_WARNING/E_NOTICE) instead raise E_DEPRECATED/E_STRICT, for PHP7 in other thread. You may consider the suggestion while I think E_WARNING is OK. Regards,
-- Yasuo Ohgaki yohgaki@ohgaki.net

Marcio Almada

11 years ago
Hi, 2015-03-05 20:08 GMT-03:00 Yasuo Ohgaki <yohgaki@ohgaki.net>:
> Hi Marcio, > > On Fri, Mar 6, 2015 at 6:36 AM, Marcio Almada <marcio.web2@gmail.com> > wrote: > >> It would be nothing new or weird. It's just a simple compile time check. >> We already "inspect" code in many other situations while compiling and a >> lot of behaviors during execution already depend on compile time checks. I >> don't think this is "too much". It might sound unusual, at first, but there >> is nothing wrong with this. > > > I really like your idea. You proved usefulness very well by your research. > My only concern is non-language construct detection by compiler. I'll vote > "yes" regardless of my suggestion if it's not an issue. >
Great, thanks.
> Zeev suggested better path for migration, not to raise errors > (E_ERROR/E_WARNING/E_NOTICE) instead raise E_DEPRECATED/E_STRICT, for PHP7 > in other thread. You may consider the suggestion while I think E_WARNING is > OK. > >
You are right about this. I'll setup a yes/no vote + a vote to decide between E_WARNING (for consistency), E_DEPRECATED or E_STRICT. For me this is just a detail but maybe it's very important to others, so better to let each voter decide upon it. Regards,
> > -- > Yasuo Ohgaki > yohgaki@ohgaki.net > >
Regards, Márcio

Patrick ALLAERT

11 years ago
Hello, Le ven. 6 mars 2015 à 00:44, Marcio Almada <marcio.web2@gmail.com> a écrit :
> > You are right about this. I'll setup a yes/no vote + a vote to decide > between E_WARNING (for consistency), E_DEPRECATED or E_STRICT. For me this > is just a detail but maybe it's very important to others, so better to let > each voter decide upon it. >
In case of language changes, shouldn't the 2/3 of majority be required at any levels? In situations like: Main feature: No/Yes Option: A, B or C My gut feeling is that it would be better to rally a 2/3 majority of people behind one of: No / Yes (A) / Yes (B) / Yes (C) in order to not dilute the importance of language changes. It would prevent accepting an important change where a lot of people agrees on a general idea but have strong opinions/arguments on implementation/details. Cheers, Patrick

Marcio Almada

11 years ago
Hi, This is a remainder that the voting for the "Strict Argument Count" RFC is scheduled to start on March 14th, so we still have this week for discussion and it's still a good time to give feedback. Thanks, Márcio 2015-03-01 20:02 GMT-03:00 Marcio Almada <marcio.web2@gmail.com>:

Patrick ALLAERT

11 years ago
Hello, Le lun. 2 mars 2015 à 00:03, Marcio Almada <marcio.web2@gmail.com> a écrit : Hi, internals
> > I'm moving the "Strict Argument Count" RFC into discussion phase: > > RFC: https://wiki.php.net/rfc/strict_argcount > PR: https://github.com/php/php-src/pull/1108 > > Many different opinions were collected during research phase and the RFC > was updated with real BC break measurements and other important sections. > So, before discussing: > > - Even if you already read the RFC in the past, read it again now. > - Don't claim **possible** massive BC breaks before read the > measurements already done. No matter how seasoned you are with PHP, real > numbers matter most than assumptions. Your measurements are welcome too. > - Try the patch. Really. > - Consider reading the use case present on this post: goo.gl/3ykdIy > - Keep discussion on topic and remember we are all trying to improve PHP > in some way :) > > Thanks, > Márcio >
I'm globally +0.5, however I have some concerns: What about constructors? Children classes may have a bigger number of arguments for their ctors than their parents. Even if not very elegant, it is possible some are passing a fixed number of arguments while constructing an object, whether that object will take it into account or not. Something like: class A { function __construct($a) } class B extends A { function __construct($a, $b) } $kind = $bool ? "A" : "B"; $object = new $kind($foo, $bar); Why aren't you using E_NOTICE? [1]: Run-time notices. Indicate that the script encountered something that could indicate an error, but could also happen in the normal course of running a script. E_DEPRECATED: -1, what is E_DEPRECATED is supposed to be removed in a future version. And that is a huge BC break if it happens. Btw, you're not mentioning in which version of PHP the support of extra parameters would be removed. E_WARNING: -1, IMHO, calling functions/methods with more arguments generally has less impact than other aspects that currently generate E_NOTICES (e.g. using undefined variables, constants,...). Using an error reporting level stronger than for those cases looks inconsistent. Cheers and thanks for the impressive work so far! Patrick [1] http://php.net/manual/en/errorfunc.constants.php

Marcio Almada

11 years ago
2015-03-10 12:31 GMT-03:00 Patrick ALLAERT <patrickallaert@php.net>:
> Hello, > > Le lun. 2 mars 2015 à 00:03, Marcio Almada <marcio.web2@gmail.com> a > écrit : > > > I'm globally +0.5, however I have some concerns: > > What about constructors? > > Children classes may have a bigger number of arguments for their ctors > than their parents. Even if not very elegant, it is possible some are > passing a fixed number of arguments while constructing an object, whether > that object will take it into account or not. > > Something like: > > class A { > function __construct($a) > } > > class B extends A { > function __construct($a, $b) > } > > $kind = $bool ? "A" : "B"; > > $object = new $kind($foo, $bar); >
I think this is somehow covered here https://wiki.php.net/rfc/strict_argcount#hassle_factor, the example is not explicit to ctors but the same principles seem to apply. Not sure you have a deeper point on it though as PHP is a weirdo and allows constructors on interfaces. Also, FYI, before we reach discussion phase, there was an idea to ignore ctors and other magic methods but you are the first person to bring it up on the ML. I'm not very inclined to ignore any other magic methods other than *__call* and *__callStatic* for now.
> > Why aren't you using E_NOTICE? >
> [1]: Run-time notices. Indicate that the script encountered something that > could indicate an error, but could also happen in the normal course of > running a script. > >
I have nothing against E_NOTICE in this case and indeed E_NOTICE seems like a good fit. I'll add it as an option.
> E_DEPRECATED: > -1, what is E_DEPRECATED is supposed to be removed in a future version. > And that is a huge BC break if it happens. Btw, you're not mentioning in > which version of PHP the support of extra parameters would be removed. > >
I was sympathetic towards E_DEPRECATED in the early stage of discussion phase but I'm not anymore. If somebody would like to E_DEPRECATE something related to argument count in the future, the this person will have to create another RFC and start the deprecation on PHP 7.1. I'm removing this option now :)
> E_WARNING: > -1, IMHO, calling functions/methods with more arguments generally has less > impact than other aspects that currently generate E_NOTICES (e.g. using > undefined variables, constants,...). Using an error reporting level > stronger than for those cases looks inconsistent. > >
I disagree with the "looks inconsistent" part. The E_WARNING option would actually be the most consistent with current PHP behavior. Ex: function fn($a, $b){} fn(1); PHP warning: Missing argument 2 for fn(), called in... If we choose E_WARNING both minimum and maximum argument count will have the same error level. BTW, in some cases an exceeding argument can be even more dangerous than a missing argument. I have no strong feelings regarding to the error level, the E_WARNING vs E_NOTICE seems legit so I'm waiting for more opinions. Cheers and thanks for the impressive work so far!
> > Patrick > > [1] http://php.net/manual/en/errorfunc.constants.php >
You're welcome. Thanks for the useful feedback. Hope we can reach a consensus on the options before the voting starts. Márcio.

Patrick ALLAERT

11 years ago
Le mar. 10 mars 2015 à 21:04, Marcio Almada <marcio.web2@gmail.com> a écrit :
> > > 2015-03-10 12:31 GMT-03:00 Patrick ALLAERT <patrickallaert@php.net>: > >> Hello, >> >> Le lun. 2 mars 2015 à 00:03, Marcio Almada <marcio.web2@gmail.com> a >> écrit : >> >> >> I'm globally +0.5, however I have some concerns: >> >> What about constructors? >> >> Children classes may have a bigger number of arguments for their ctors >> than their parents. Even if not very elegant, it is possible some are >> passing a fixed number of arguments while constructing an object, whether >> that object will take it into account or not. >> >> Something like: >> >> class A { >> function __construct($a) >> } >> >> class B extends A { >> function __construct($a, $b) >> } >> >> $kind = $bool ? "A" : "B"; >> >> $object = new $kind($foo, $bar); >> > > I think this is somehow covered here > https://wiki.php.net/rfc/strict_argcount#hassle_factor, the example is > not explicit to ctors but the same principles seem to apply. Not sure you > have a deeper point on it though as PHP is a weirdo and allows constructors > on interfaces. > > Also, FYI, before we reach discussion phase, there was an idea to ignore > ctors and other magic methods but you are the first person to bring it up > on the ML. I'm not very inclined to ignore any other magic methods other > than *__call* and *__callStatic* for now. >
No deep point, just wanted to bring your attention on it.
> > >> E_WARNING: >> -1, IMHO, calling functions/methods with more arguments generally has >> less impact than other aspects that currently generate E_NOTICES (e.g. >> using undefined variables, constants,...). Using an error reporting level >> stronger than for those cases looks inconsistent. >> > > I disagree with the "looks inconsistent" part. The E_WARNING option would > actually be the most consistent with current PHP behavior. Ex: > > function fn($a, $b){} > fn(1); > PHP warning: Missing argument 2 for fn(), called in... > > If we choose E_WARNING both minimum and maximum argument count will have > the same error level. BTW, in some cases an exceeding argument can be even > more dangerous than a missing argument. >
You have a (debatable) point :) It depends according to what aspect of PHP your are comparing it with. Greping zend_error(E_NOTICE,.*) in the code I had the feeling that already many notices where the sign of a bigger problem than when passing extra parameters, hence why I suggested E_NOTICE. My consistency argument was therefore "severity based".
> I have no strong feelings regarding to the error level, the E_WARNING vs > E_NOTICE seems legit so I'm waiting for more opinions. >
I wouldn't -1 for E_WARNING because of your extra arguments (sorry for the pun ;), someone had to do it!). Still in favor of E_NOTICE though. Patrick

Marcio Almada

11 years ago
Hi 2015-03-11 6:50 GMT-03:00 Patrick ALLAERT <patrickallaert@php.net>:
> Le mar. 10 mars 2015 à 21:04, Marcio Almada <marcio.web2@gmail.com> a > écrit : > >> >> >> 2015-03-10 12:31 GMT-03:00 Patrick ALLAERT <patrickallaert@php.net>: >> >>> Hello, >>> >>> Le lun. 2 mars 2015 à 00:03, Marcio Almada <marcio.web2@gmail.com> a >>> écrit : >>> >>> >>> I'm globally +0.5, however I have some concerns: >>> >>> What about constructors? [...] >>> >> I think this is somehow covered here >>> https://wiki.php.net/rfc/strict_argcount#hassle_factor, the example is >>> not explicit to ctors but the same principles seem to apply. Not sure you >>> have a deeper point on it though as PHP is a weirdo and allows constructors >>> on interfaces. >>> >>> Also, FYI, before we reach discussion phase, there was an idea to ignore >>> ctors and other magic methods but you are the first person to bring it up >>> on the ML. I'm not very inclined to ignore any other magic methods other >>> than *__call* and *__callStatic* for now. >>> >> > No deep point, just wanted to bring your attention on it. > >> >> >>> E_WARNING: >>> -1, IMHO, calling functions/methods with more arguments generally has >>> less impact than other aspects that currently generate E_NOTICES (e.g. >>> using undefined variables, constants,...). Using an error reporting level >>> stronger than for those cases looks inconsistent. >>> >> >> I disagree with the "looks inconsistent" part. The E_WARNING option would >> actually be the most consistent with current PHP behavior. Ex: >> >> function fn($a, $b){} >> fn(1); >> PHP warning: Missing argument 2 for fn(), called in... >> >> If we choose E_WARNING both minimum and maximum argument count will have >> the same error level. BTW, in some cases an exceeding argument can be even >> more dangerous than a missing argument. >> > > You have a (debatable) point :) > > It depends according to what aspect of PHP your are comparing it with. > Greping zend_error(E_NOTICE,.*) in the code I had the feeling that already > many notices where the sign of a bigger problem than when passing extra > parameters, hence why I suggested E_NOTICE. > > My consistency argument was therefore "severity based". >
Ok, that's a good POV. It just turns out that the severity may vary from case to case.
> I have no strong feelings regarding to the error level, the E_WARNING vs >> E_NOTICE seems legit so I'm waiting for more opinions. >> > > I wouldn't -1 for E_WARNING because of your extra arguments (sorry for the > pun ;), someone had to do it!). Still in favor of E_NOTICE though. > >
That's great to know. Looks like E_WARNING vs E_NOTICE vs E_STRICT seems to be good setup even though I still don't like to have 3 options though.
> Patrick >
Thanks, Márcio

Stas Malyshev

11 years ago
Hi!
> - Even if you already read the RFC in the past, read it again now. > - Don't claim **possible** massive BC breaks before read the > measurements already done. No matter how seasoned you are with PHP, real > numbers matter most than assumptions. Your measurements are welcome too.
I'd say your own data - 4597 warnings on 27 different cases of calls - is quite fitting the "massive BC break" category. If I had some app and had to evaluate the upgrade to PHP 7 and would see that it produces thousands of errors and I have to patch dozens of separate places to fix it (your data doesn't say if the sources of the calls are the same, so I take the best interpretation - that both source and target are the same) - I would postpone the upgrade as far as possible, because it looks like a huge disruption. Additionally, ZF2 tests couldn't even run - and from this you conclude that your patch is fine. I conclude that we already have a big problem, and making it worse is not a good idea. If we do it couple of more times, we'd probably get people to run PHP 7 somewhere in 2025. Additionally, I feel bad about magic features that change depending on which code is inside the function. That doesn't feel like a good design. Additionally, there is more than variadic functions which may use extra parameters. Such as dynamic dispatch: function getVariables($vars, $arg) { foreach($vars as $var) { $getter = "get$var"; $result[] = $this->$getter($arg); } return $result; } grossly simplified of course. Now imagine some getters need the arg and some don't. I can't write this code anymore without either: 1. Adding argument to all getters even though they don't need it (unclean and confusing) 2. Checking how many arguments each getter needs and modifying the loop accordingly (even more confusing)
> - Try the patch. Really. > - Consider reading the use case present on this post: goo.gl/3ykdIy
The case you bring there looks artificially constructed to support this RFC. If your unit tests are so bad as to not actually check that the functions like createUser() do actually create the user you asked to create and don't just drop information on the floor, and your code refactoring practices are so bad as to allow people just drop arguments from public APIs and replace them with other arguments, and all this without checks or typing - parameter counts is not what is going to save you. Your own example would break if both changes are done at the same time (if you can do either of them, no reason why you can't do both).
-- Stas Malyshev smalyshev@gmail.com

Marcio Almada

11 years ago
Hi, 2015-03-10 21:31 GMT-03:00 Stanislav Malyshev <smalyshev@gmail.com>:
> Hi! > > > - Even if you already read the RFC in the past, read it again now. > > - Don't claim **possible** massive BC breaks before read the > > measurements already done. No matter how seasoned you are with PHP, > real > > numbers matter most than assumptions. Your measurements are welcome > too. > > I'd say your own data - 4597 warnings on 27 different cases of calls - > is quite fitting the "massive BC break" category. If I had some app and > had to evaluate the upgrade to PHP 7 and would see that it produces > thousands of errors and I have to patch dozens of separate places to fix > it (your data doesn't say if the sources of the calls are the same, so I > take the best interpretation - that both source and target are the same) > - I would postpone the upgrade as far as possible, because it looks like > a huge disruption. >
Have you read the rest of the paragraph you distorted? Because you just took a slice of the paragraph, presented it out of context like if everybody will just buy your malformed misinterpretation without go check the RFC and see: [...] Running the test suite resulted in a slay of *4.597* warnings directly related to the proposed RFC. *But* after some heuristics it was noticeable that most warnings had a common cause. I parsed the output file to remove duplicated warnings and the result was a derisive number of *only 27 unique warnings*. You can see the condensed list of unique warnings below: [...] After checking case by case among the *27 unique* warnings found, the conclusion is that *all of them* are a result of *human mistake* from some code refactory that left function calls with residual parameters behind. This could be fixed in less than 1 hour. [...] The wordpress test suite is basically a bunch of integration tests. If you have a single warning it will repeat 4 thousand times, if you fix this warning it disappears from all 4 thousand tests. I even parsed the output file to prove that it's only 27 warnings, not the apparent 4.597 warnings you decided to quote out of context. It was clearly proven that the BC break is very isolated even in a massive codebase like wordpress. Fixing 27 function calls won't impede Wordpress or anyone to migrate to PHP7. It's an easy fix that could prevent nasty dangerous bugs later.
> Additionally, ZF2 tests couldn't even run - and from this you conclude > that your patch is fine. I conclude that we already have a big problem, > and making it worse is not a good idea. If we do it couple of more > times, we'd probably get people to run PHP 7 somewhere in 2025. > >
The ZF2 test suite couldn't run because of fatal errors caused by PHP7 incompatibilities or bugs that caused segfaults. Nothing related to the RFC itself. Hope you don't expect to prove a point just by saying that an isolated BC break will make the PHP7 migration "much worse" based solely on your instincts about a test suite that didn't even run - for unrelated reasons to the RFC proposal - (look at the other test suites that ran fine). I measured the BC breaks responsibly before claim anything. Try to do the same. That's not enough effort of your part on a mailing list discussion, sorry.
> Additionally, I feel bad about magic features that change depending on > which code is inside the function. That doesn't feel like a good design. >
If compilation is "magic"... we are all magicians here. Calling the implementation "magic" won't change the fact that it's just a compilation check :D
> Additionally, there is more than variadic functions which may use extra > parameters. Such as dynamic dispatch: > > function getVariables($vars, $arg) { > foreach($vars as $var) { > $getter = "get$var"; > $result[] = $this->$getter($arg); > } > return $result; > } > > grossly simplified of course. Now imagine some getters need the arg and > some don't. I can't write this code anymore without either: > 1. Adding argument to all getters even though they don't need it > (unclean and confusing) > 2. Checking how many arguments each getter needs and modifying the loop > accordingly (even more confusing) > >
If you plan to use methods|functions interchangeably then you should make them compatible from now on. That's one of the points of the RFC. It's not "unclean". BTW, the current PHP silent behavior should be considered even more confusing otherwise we wouldn't have these measurements: https://wiki.php.net/rfc/strict_argcount#bc_breaks_on_the_real_world It basically means that even experienced package maintainers are shooting on their own foot and they are using code evaluation tools, static analyzers, etc. Now imagine the common PHP programmer.
> > - Try the patch. Really. > > - Consider reading the use case present on this post: goo.gl/3ykdIy > > The case you bring there looks artificially constructed to support this > RFC. If your unit tests are so bad as to not actually check that the > functions like createUser() do actually create the user you asked to > create and don't just drop information on the floor, and your code > refactoring practices are so bad as to allow people just drop arguments > from public APIs and replace them with other arguments, and all this > without checks or typing - parameter counts is not what is going to save > you. Your own example would break if both changes are done at the same > time (if you can do either of them, no reason why you can't do both). > >
There is nothing bad about using examples to illustrate something, just because the example is "artificial" it doesn't mean it's disconnected from reality. The example on goo.gl/3ykdIy is far from being a lie, it's indeed an accurate example. You can't claim that I artificially built the test suites of a lot of open source projects just to "support the RFC". The test suites (and the code being tested) grew quite "organically", I just run them to do some measurements: - and there it is: the same patterns from the "artificial" example were detected numerous times "on the wild".
> -- > Stas Malyshev > smalyshev@gmail.com >
Márcio

Stas Malyshev

11 years ago
Hi!
> related to the proposed RFC. *But* after some heuristics it was > noticeable that most warnings had a common cause. I parsed the output
It doesn't matter if it has common cause or not. If I have a system of Wordpress-like size, I'm bound to get a lot of failures, that's what it is telling me. And 27 separate failures are non-negligible number.
> some code refactory that left function calls with residual parameters > behind. This could be fixed in less than 1 hour.
I think you are severely underestimating the cost of fixing it. Fixing bugs is not only replacing the bytes.
> The ZF2 test suite couldn't run because of fatal errors caused by PHP7 > incompatibilities > or bugs that caused segfaults. Nothing related to the RFC itself.
We do not know if something there is related to RFC or not. That's not evidence that RFC is OK, that's evidence that a) we could not obtain the information and b) we already have a BC problem so severe that we can not run ZF2, and probably not easily fixable (since, I assume, if it were easily fixable you'd have done so). That's why I am reluctant to add more BC breaks, especially ones that bring no new capabilities but just add more error messages. Each new BC break adds migration barriers, and in my opinion, it's not even linear.
> If compilation is "magic"... we are all magicians here. Calling the > implementation "magic" > won't change the fact that it's just a compilation check :D
I didn't say compilation is "magic", you are strawmanning here. What I said is that having different behavior of the compiler depending on specific function calls made by the function being compiled is pretty rare (not only in PHP but in other languages too) and non-obvious, and thus suspicious.
> If you plan to use methods|functions interchangeably then you should > make them compatible > from now on. That's one of the points of the RFC. It's not "unclean".
You basically just choose to ignore my argument here, recognizing that I correctly identified the case where BC break would happen but saying you don't care about it. Other people that do use such code may care. And yes, I consider adding unnecessary parameters to a function just to satisfy some warning "unclean".
> BTW, the current PHP silent behavior should be considered even more > confusing otherwise we > wouldn't have these measurements: > > https://wiki.php.net/rfc/strict_argcount#bc_breaks_on_the_real_world
I'm not sure how these prove current behavior is "confusing". Yes, there are bugs in the code, and I'm sure this feature will expose some of those (previously harmless) bugs. At cost of the BC break. And don't think a handful of libraries represent the vast universe of PHP code, most of which we can't even see because it's not published anywhere.
> It basically means that even experienced package maintainers are > shooting on their own foot and > they are using code evaluation tools, static analyzers, etc. Now imagine > the common PHP programmer.
Yes, experienced programmers make bugs. Inexperienced programmers make bugs too. Nobody ever claimed otherwise. The point is not about that, the point is in this case the cure may be worse than the disease.
> You can't claim that I artificially built the test suites of a lot of > open source > projects just to "support the RFC". The test suites (and the code being
Coincidentally, I also didn't claim that. Should we go and enumerate more things that I can't claim and actually didn't, or we are ready to proceed to discussing things I actually did claim?
> - and there it is: the same patterns from the "artificial" example were > detected > numerous times "on the wild".
No, your example talks about something completely different - removing a parameter used by the code and the tests completely missing that even though the API requires that parameter to be used, and then adding another parameter in place of that previous parameter and giving it completely different meaning and the tests missing it again. If some framework in the wild has such code changes and such tests, they need to seriously upgrade their testing game.
-- Stas Malyshev smalyshev@gmail.com

Marcio Almada

11 years ago
Hi 2015-03-11 1:49 GMT-03:00 Stanislav Malyshev <smalyshev@gmail.com>:
> Hi! > > > related to the proposed RFC. *But* after some heuristics it was > > noticeable that most warnings had a common cause. I parsed the output > > It doesn't matter if it has common cause or not. If I have a system of > Wordpress-like size, I'm bound to get a lot of failures, that's what it > is telling me. And 27 separate failures are non-negligible number. > >
It's just 27 function calls to fix, and you only need to remove a few arguments that are being ignored because the functions are not taking them into account... I don't think it has a big cost to fix. I fixed them myself in less than one hour! This won't be an E_INSANELY frequent warning and fix is straightforward, as the warning message is usually very explicit about where the call happens and where the function was declared.
> > some code refactory that left function calls with residual parameters > > behind. This could be fixed in less than 1 hour. > > I think you are severely underestimating the cost of fixing it. Fixing > bugs is not only replacing the bytes. > >
Same as up there.
> > The ZF2 test suite couldn't run because of fatal errors caused by PHP7 > > incompatibilities > > or bugs that caused segfaults. Nothing related to the RFC itself. > > We do not know if something there is related to RFC or not. That's not > evidence that RFC is OK, that's evidence that a) we could not obtain the > information and b) we already have a BC problem so severe that we can > not run ZF2, and probably not easily fixable (since, I assume, if it > were easily fixable you'd have done so). >
That's why I am reluctant to
> add more BC breaks, especially ones that bring no new capabilities but > just add more error messages. Each new BC break adds migration barriers, > and in my opinion, it's not even linear. > >
I really get your concerns, but this RFC will not increase the migration barrier as much as you are thinking (not even close). Just look at the other projects that I tested too ;) That's why I tried to measure the BC breaks, I was expecting more BC breaks but it turned out surprisingly sparse breaks and most are potential bugs. When you say "the RFC brings no new capabilities" I disagree with you. The RFC makes PHP safer and easier for people to catch problems, that's a good capability.
> If compilation is "magic"... we are all magicians here. Calling the > > implementation "magic" > > won't change the fact that it's just a compilation check :D > > I didn't say compilation is "magic", you are strawmanning here. What I > said is that having different behavior of the compiler depending on > specific function calls made by the function being compiled is pretty > rare (not only in PHP but in other languages too) and non-obvious, and > thus suspicious. > >
The bad design is on PHP which unfortunately decided that variadic functions should be determined by func_get_args usage. I'm just dealing with that oddity by doing a compile time check. It's not "suspicious". It's predictable and reliable. We check function signatures, we check return by ref and many other things. If you think it really straight there is nothing really shocking on the proposed solution. You might not like the RFC, but I don't think this is a point for concerns.
> > If you plan to use methods|functions interchangeably then you should > > make them compatible > > from now on. That's one of the points of the RFC. It's not "unclean". > > You basically just choose to ignore my argument here, recognizing that I > correctly identified the case where BC break would happen but saying you > don't care about it. Other people that do use such code may care. > And yes, I consider adding unnecessary parameters to a function just to > satisfy some warning "unclean". > >
I'll sincerely try to give some more thought on this subject. I don't think it's 100% necessary but I'll try to see if there is a possible alternative for this case.
> > BTW, the current PHP silent behavior should be considered even more > > confusing otherwise we > > wouldn't have these measurements: > > > > https://wiki.php.net/rfc/strict_argcount#bc_breaks_on_the_real_world > > I'm not sure how these prove current behavior is "confusing". Yes, there > are bugs in the code, and I'm sure this feature will expose some of > those (previously harmless) bugs.
"Harmless bugs" that one day might get promoted to heartbleed status on some widely used package or CMS out there if you don't kill them ;)
> At cost of the BC break. And don't > think a handful of libraries represent the vast universe of PHP code, > most of which we can't even see because it's not published anywhere. >
Unfortunately we can't run each in house code out there to prove something or not, but I had to test drive the patch and picked a diverse number of packages and FOSS projects so everybody can repeat the tests too. There is no other way to prove usefulness and measure BCB other than running the test suites (it's all green) and running real projects that anyone can have access to reproduce the tests. And you also have the patch to test things yourself if you really have the willpower. If you think that the current methodology used for measurements is not valid, I'm afraid there is nothing else I can do to make it better.
> > It basically means that even experienced package maintainers are > > shooting on their own foot and > > they are using code evaluation tools, static analyzers, etc. Now imagine > > the common PHP programmer. > > Yes, experienced programmers make bugs. Inexperienced programmers make > bugs too. Nobody ever claimed otherwise. The point is not about that, > the point is in this case the cure may be worse than the disease. > > > You can't claim that I artificially built the test suites of a lot of > > open source > > projects just to "support the RFC". The test suites (and the code being > > Coincidentally, I also didn't claim that. Should we go and enumerate > more things that I can't claim and actually didn't, or we are ready to > proceed to discussing things I actually did claim? > >
You said that the example given was "artificially built" to prove a point. I just said that I couldn't be artificially building the test suites from all the FOSS projects tested as sampling. And the example given was based on the experience of testing these projects and getting the results (it's far from "artificial").
> > - and there it is: the same patterns from the "artificial" example were > > detected > > numerous times "on the wild". > > No, your example talks about something completely different - removing a > parameter used by the code and the tests completely missing that even > though the API requires that parameter to be used, and then adding > another parameter in place of that previous parameter and giving it > completely different meaning and the tests missing it again. If some > framework in the wild has such code changes and such tests, they need to > seriously upgrade their testing game. > >
I don't want to sound repetitive, but that's exactly what's happening on some of the tested code. Parameters were removed and nothing warns about the wrong method|function calls because PHP happily swallow the arguments... until somebody create another argument that will suddenly take the dormant values being passed into account - bug.
> -- > Stas Malyshev > smalyshev@gmail.com >
Márcio

Marc Bennewitz

11 years ago
Hi Stas,
> Stanislav Malyshev <smalyshev@gmail.com> hat am 11. März 2015 um 05:49 > geschrieben: > > > Hi! > > > related to the proposed RFC. *But* after some heuristics it was > > noticeable that most warnings had a common cause. I parsed the output > > It doesn't matter if it has common cause or not. If I have a system of > Wordpress-like size, I'm bound to get a lot of failures, that's what it > is telling me. And 27 separate failures are non-negligible number.
This RFC doesn't add failures. It only makes failures visible. The failures are present without this this patch too but invisible.
> > some code refactory that left function calls with residual parameters > > behind. This could be fixed in less than 1 hour. > > I think you are severely underestimating the cost of fixing it. Fixing > bugs is not only replacing the bytes.
Same as above. The Bugs are there with and without this RFC.
> > The ZF2 test suite couldn't run because of fatal errors caused by PHP7 > > incompatibilities > > or bugs that caused segfaults. Nothing related to the RFC itself. > > We do not know if something there is related to RFC or not. That's not > evidence that RFC is OK, that's evidence that a) we could not obtain the > information and b) we already have a BC problem so severe that we can > not run ZF2, and probably not easily fixable (since, I assume, if it > were easily fixable you'd have done so). That's why I am reluctant to > add more BC breaks, especially ones that bring no new capabilities but > just add more error messages. Each new BC break adds migration barriers, > and in my opinion, it's not even linear.
The current fatals are not related to this patch. Sure, because of this it's unknown if there are other errors related to this patch but it doesn't make this RFC better or worse. In my opinion this RFC only introduces a very very small BC break and only in the case warnings are thrown as exceptions by the user (as in composer). This RFC only makes currently invisible failures visible.
> > If compilation is "magic"... we are all magicians here. Calling the > > implementation "magic" > > won't change the fact that it's just a compilation check :D > > I didn't say compilation is "magic", you are strawmanning here. What I > said is that having different behavior of the compiler depending on > specific function calls made by the function being compiled is pretty > rare (not only in PHP but in other languages too) and non-obvious, and > thus suspicious.
First of all please be consistent with your meaning ... on discussion about "$this from incompatible context" you liked to change a function callable or not if the function contains $this or not. In the normal case also say the function body should not define anything about how a function is callable but in this case the functions func_get_arg[s] already do this.
> > BTW, the current PHP silent behavior should be considered even more > > confusing otherwise we > > wouldn't have these measurements: > > > > https://wiki.php.net/rfc/strict_argcount#bc_breaks_on_the_real_world > > I'm not sure how these prove current behavior is "confusing". Yes, there > are bugs in the code, and I'm sure this feature will expose some of > those (previously harmless) bugs. At cost of the BC break. And don't > think a handful of libraries represent the vast universe of PHP code, > most of which we can't even see because it's not published anywhere.
What do you mean here? Again, introducing a warning isn't a real BC-break and as you says there are bugs it helps nobody ignoring them. If you think so you can do that with all of your own bugs by ignoring warnings/notices.
> <snip>
Marc

Lester Caine

11 years ago
On 11/03/15 09:05, wp12173047-156224 wp12173047-156224 wrote:
>>> BTW, the current PHP silent behavior should be considered even more >>> > > confusing otherwise we >>> > > wouldn't have these measurements: >>> > > >>> > > https://wiki.php.net/rfc/strict_argcount#bc_breaks_on_the_real_world >> > >> > I'm not sure how these prove current behavior is "confusing". Yes, there >> > are bugs in the code, and I'm sure this feature will expose some of >> > those (previously harmless) bugs. At cost of the BC break. And don't >> > think a handful of libraries represent the vast universe of PHP code, >> > most of which we can't even see because it's not published anywhere. > What do you mean here? > > Again, introducing a warning isn't a real BC-break and as you says there are > bugs it helps nobody ignoring them. If you think so you can do that with all of > your own bugs by ignoring warnings/notices.
I must say I was a little confused when I first saw this given the number of errors I got when addressing the E_STRICT ones on existing code. One ended up creating multiple functions with different arguments to get a clean set of code, but I had perhaps not even realised that adding extra arguments was not flagged. In practice my IDE setup is flagging warnings where variables are not used, and it seems that perhaps I'm in a 'safe place' because of the whole package rather than a particular 'bug' in PHP. Most of the examples being shown are examples of simple bad programming practice that needs fixing anyway, and I would expect a proper code review to have picked them up, so don't see that adding the check in PHP is essential. It would however be a useful addition but in the E_STRICT category ... not that I want to maintain that, but being able to ignore those errors until such time as it is appropriate to fix them. The poorly checked code is the problem rather than PHP here, and I'm with Stas that it's probably being a bit heavy handed at this point in the PHP7 process. Actually I would be rather annoyed if the problems demonstrated have not already been cleaned up already? A tool to identify problems and get them cleaned before the main test is switched on helps everybody?
-- Lester Caine - G8HFL ----------------------------- Contact - http://lsces.co.uk/wiki/?page=contact L.S.Caine Electronic Services - http://lsces.co.uk EnquirySolve - http://enquirysolve.com/ Model Engineers Digital Workshop - http://medw.co.uk Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

Marcio Almada

11 years ago
2015-03-11 6:27 GMT-03:00 Lester Caine <lester@lsces.co.uk>:
> On 11/03/15 09:05, wp12173047-156224 wp12173047-156224 wrote: > >>> BTW, the current PHP silent behavior should be considered even more > >>> > > confusing otherwise we > >>> > > wouldn't have these measurements: > >>> > > > >>> > > > https://wiki.php.net/rfc/strict_argcount#bc_breaks_on_the_real_world > >> > > >> > I'm not sure how these prove current behavior is "confusing". Yes, > there > >> > are bugs in the code, and I'm sure this feature will expose some of > >> > those (previously harmless) bugs. At cost of the BC break. And don't > >> > think a handful of libraries represent the vast universe of PHP code, > >> > most of which we can't even see because it's not published anywhere. > > What do you mean here? > > > > Again, introducing a warning isn't a real BC-break and as you says there > are > > bugs it helps nobody ignoring them. If you think so you can do that with > all of > > your own bugs by ignoring warnings/notices. > > I must say I was a little confused when I first saw this given the > number of errors I got when addressing the E_STRICT ones on existing > code. One ended up creating multiple functions with different arguments > to get a clean set of code, but I had perhaps not even realised that > adding extra arguments was not flagged.
A lot of people don't know it too, and they are shooting themselves on the foot without realizing it because PHP is currently silent about it.
> In practice my IDE setup is > flagging warnings where variables are not used, and it seems that > perhaps I'm in a 'safe place' because of the whole package rather than a > particular 'bug' in PHP. > >
Your IDE is a tool you are using to be safe from the current PHP misleading behavior. It's fine to use tooling, but that's a basic feature that shouldn't require special tooling.
> Most of the examples being shown are examples of simple bad programming > practice that needs fixing anyway, and I would expect a proper code > review to have picked them up, so don't see that adding the check in PHP > is essential. It would however be a useful addition but in the E_STRICT > category ... not that I want to maintain that, but being able to ignore > those errors until such time as it is appropriate to fix them.
I think this is a valid argument to keep the E_STRICT error level option for the secondary voting. That's a very useful information, thanks :)
> The > poorly checked code is the problem rather than PHP here, and I'm with > Stas that it's probably being a bit heavy handed at this point in the > PHP7 process. Actually I would be rather annoyed if the problems > demonstrated have not already been cleaned up already?
The examples are not "poorly checked" code, they were retrieved on widely used packages we all rely, directly or indirectly. Both code and tests used in the measurements are maintained by experienced people and yet the problems found a way to lurk (it's not a fault on the maintainers side). I'd say it's enough information to improve the language, not to require that people should manually check their code more or use tool A or B.
> A tool to > identify problems and get them cleaned before the main test is switched > on helps everybody? >
-- > Lester Caine - G8HFL > ----------------------------- > Contact - http://lsces.co.uk/wiki/?page=contact > L.S.Caine Electronic Services - http://lsces.co.uk > EnquirySolve - http://enquirysolve.com/ > Model Engineers Digital Workshop - http://medw.co.uk > Rainbow Digital Media - http://rainbowdigitalmedia.co.uk > > -- > PHP Internals - PHP Runtime Development Mailing List > To unsubscribe, visit: http://www.php.net/unsub.php > > Thanks, Márcio

Patrick ALLAERT

11 years ago
Le mer. 11 mars 2015 à 22:44, Marcio Almada <marcio.web2@gmail.com> a écrit :
> 2015-03-11 6:27 GMT-03:00 Lester Caine <lester@lsces.co.uk>: > > > On 11/03/15 09:05, wp12173047-156224 wp12173047-156224 wrote: > > > Most of the examples being shown are examples of simple bad programming > > practice that needs fixing anyway, and I would expect a proper code > > review to have picked them up, so don't see that adding the check in PHP > > is essential. It would however be a useful addition but in the E_STRICT > > category ... not that I want to maintain that, but being able to ignore > > those errors until such time as it is appropriate to fix them. >
I don't really see how this favors E_STRICT over E_NOTICE as any of this type of errors can be displayed/hidden independently.
> I think this is a valid argument to keep the E_STRICT error level option > for the secondary voting. > That's a very useful information, thanks :) >
It also depends on your perception of E_STRICT. This level has been introduced in 5.0 without being part of E_ALL in order to, among other things, avoid too much pain in the *** while migrating from 4.x to 5.x. As of 5.4, E_ALL contains E_STRICT and the difference between E_STRICT and E_NOTICE/E_WARNING is certainly not in terms of severity. Using an undefined variable or property => notice. Trying to get property of non-object => notice. Use of undefined constant => notice For this reason, I think we should use the standard notice/warning/error levels as much as possible. You may take a look at Nikita's "Reclassify E_STRICT RFC" for more info about it. https://wiki.php.net/rfc/reclassify_e_strict Cheers, Patrick

Lester Caine

11 years ago
On 13/03/15 09:02, Patrick ALLAERT wrote:
> It also depends on your perception of E_STRICT. This level has been > introduced in 5.0 without being part of E_ALL in order to, among other > things, avoid too much pain in the *** while migrating from 4.x to 5.x. > As of 5.4, E_ALL contains E_STRICT and the difference between E_STRICT > and E_NOTICE/E_WARNING is certainly not in terms of severity. > Using an undefined variable or property => notice. > Trying to get property of non-object => notice. > Use of undefined constant => notice > > For this reason, I think we should use the standard notice/warning/error > levels as much as possible. You may take a look at Nikita's "Reclassify > E_STRICT RFC" for more info about it. > https://wiki.php.net/rfc/__reclassify_e_strict > <https://wiki.php.net/rfc/reclassify_e_strict>
I think the main point here is just like the PHP4->5 conversion path, SOME areas that need upgrading should be flagged by default while others should be able to be hidden until they need to be addressed. Perhaps E_STRICT7 off by default, but if all of the 4->5 conversion stuff is now reclassified then E_STRICT should be available to serve the same purpose it did back then?
-- Lester Caine - G8HFL ----------------------------- Contact - http://lsces.co.uk/wiki/?page=contact L.S.Caine Electronic Services - http://lsces.co.uk EnquirySolve - http://enquirysolve.com/ Model Engineers Digital Workshop - http://medw.co.uk Rainbow Digital Media - http://rainbowdigitalmedia.co.uk

Patrick ALLAERT

11 years ago
Le ven. 13 mars 2015 à 14:39, Lester Caine <lester@lsces.co.uk> a écrit :
> On 13/03/15 09:02, Patrick ALLAERT wrote: > > It also depends on your perception of E_STRICT. This level has been > > introduced in 5.0 without being part of E_ALL in order to, among other > > things, avoid too much pain in the *** while migrating from 4.x to 5.x. > > As of 5.4, E_ALL contains E_STRICT and the difference between E_STRICT > > and E_NOTICE/E_WARNING is certainly not in terms of severity. > > Using an undefined variable or property => notice. > > Trying to get property of non-object => notice. > > Use of undefined constant => notice > > > > For this reason, I think we should use the standard notice/warning/error > > levels as much as possible. You may take a look at Nikita's "Reclassify > > E_STRICT RFC" for more info about it. > > https://wiki.php.net/rfc/__reclassify_e_strict > > <https://wiki.php.net/rfc/reclassify_e_strict> > > I think the main point here is just like the PHP4->5 conversion path, > SOME areas that need upgrading should be flagged by default while others > should be able to be hidden until they need to be addressed. Perhaps > E_STRICT7 off by default, but if all of the 4->5 conversion stuff is now > reclassified then E_STRICT should be available to serve the same purpose > it did back then? >
This is the subject I discussed this morning in the "Reclassify E_STRICT notices" thread: http://marc.info/?l=php-internals&m=142623927703931 Yet another level would probably be a mistake. So I wish we could try working on a way to easier the conversion path using a pattern that can be repeated over the time.

Marcio Almada

11 years ago
Hi 2015-03-13 6:02 GMT-03:00 Patrick ALLAERT <patrickallaert@php.net>:
> Le mer. 11 mars 2015 à 22:44, Marcio Almada <marcio.web2@gmail.com> a > écrit : > >> 2015-03-11 6:27 GMT-03:00 Lester Caine <lester@lsces.co.uk>: >> >> > On 11/03/15 09:05, wp12173047-156224 wp12173047-156224 wrote: >> >> > Most of the examples being shown are examples of simple bad programming >> > practice that needs fixing anyway, and I would expect a proper code >> > review to have picked them up, so don't see that adding the check in PHP >> > is essential. It would however be a useful addition but in the E_STRICT >> > category ... not that I want to maintain that, but being able to ignore >> > those errors until such time as it is appropriate to fix them. >> > > I don't really see how this favors E_STRICT over E_NOTICE as any of this > type of errors can be displayed/hidden independently. > > >> I think this is a valid argument to keep the E_STRICT error level option >> for the secondary voting. >> That's a very useful information, thanks :) >> > > It also depends on your perception of E_STRICT. This level has been > introduced in 5.0 without being part of E_ALL in order to, among other > things, avoid too much pain in the *** while migrating from 4.x to 5.x. > As of 5.4, E_ALL contains E_STRICT and the difference between E_STRICT and > E_NOTICE/E_WARNING is certainly not in terms of severity. > Using an undefined variable or property => notice. > Trying to get property of non-object => notice. > Use of undefined constant => notice > > For this reason, I think we should use the standard notice/warning/error > levels as much as possible. You may take a look at Nikita's "Reclassify > E_STRICT RFC" for more info about it. > https://wiki.php.net/rfc/reclassify_e_strict > > Cheers, > Patrick >
I talked to Nikita earlier today, in order to try to align the strict arg count RFC with https://wiki.php.net/rfc/reclassify_e_strict and the conclusion was the following: It's a good thing to reclassify E_STRICT as this simplifies PHP error model and resolves the currently unclear role of strict standards notices. As a supporter of Nikita's idea, I'm removing E_STRICT from the voting options. Consider this my collaboration to help to unifiy the error level model and go a bit farther from the current "error level buffet" state we got ourselves historically, on PHP. This leaves us with E_WARNING vs E_NOTICE and I'm sufficiently comfortable to allow a secondary voting between these two error levels. Thanks, Márcio

Marco Pivetta

11 years ago
Hi Marcio/internals, I just went through the RFC again (didn't check the discussion since our last chat about it in Room 11). The section about '"Flexible" Interface Implementations' mentions the interface as an 'an acceptable “PHPism”'. This is not just an "acceptable PHPism", it's a "life saving PHPism". To give you an idea of the amount of code that was saved from a massive refactoring when ZF1 => ZF2 code was ported by current ZF2 users, think about all web apps that have form contextual validation. Guess what? Almost all web applications have forms, and almost all apps have something like a "confirm password" form. Yep, *ALL* ZF2 apps are affected by this RFC, and it will likely prevent an upgrade to PHP7 in the near future unless our users upgrade to ZF3, which will be PHP 5.5 based, and therefore without variadic arguments. This is not an interface that I'm proud of, but it is an interface that saved a lot of time/money to a lot of users, and that allows flexible handling of contextual validation where we didn't foresee one upfront (the interface is indeed *wrong*, but we can't fix it in a stable release). Until all of the framework and all of the users have migrated to variadic interfaces, this particular RFC is a problem. Getting this change into PHP7 would mean having a major part of the framework community having a real hard time in upgrading. Additionally to all these worries of mine, consider that mistakes like the one with the `ValidatorInterface` may happen again in the future, and this "PHPism" is actually saving our asses out there. Don't get rid of it for the sake of clean code, because these "dirty hacks" have already proven themselves REALLY useful. Marco Pivetta http://twitter.com/Ocramius http://ocramius.github.com/ On 14 March 2015 at 00:37, Marcio Almada <marcio.web2@gmail.com> wrote: