Everything
php
Learn Community Releases The PHP Project
Home » Project » Newsgroups » php.internals » [RFC] [Accepted] Replace "Missing argument" warning with "Too few arguments" exception »

[RFC] [Accepted] Replace "Missing argument" warning with "Too few arguments" exception

php.internals

Dmitry Stogov

10 years ago
Hi internals, The RFC https://wiki.php.net/rfc/too_few_args<https://wiki.php.net/rfc/t> was accepted by 39 against 11, and the corresponding patch was merged into master. Thanks. Dmitry.

Pierre Joye

10 years ago
Hi Dmitry I am sorry but I have to ask to wait before merging it. It is definitely not clear that: . The rfc was valid to begin with due to the short discussion time . This BC is acceptable for 7.x I think things like things can be prevented by following the relatively simple rfc process and having a more clear way to define what is acceptable as BC. I think the RMs should step in here. Thanks, Pierre On Jun 16, 2016 1:12 PM, "Dmitry Stogov" <dmitry@zend.com> wrote:

Dmitry Stogov

10 years ago
Hi Pierre, On 06/16/2016 09:18 AM, Pierre Joye wrote:
> > Hi Dmitry > > I am sorry but I have to ask to wait before merging it. >
Sorry, but this is already merged.
> > It is definitely not clear that: > > . The rfc was valid to begin with due to the short discussion time > . This BC is acceptable for 7.x > > I think things like things can be prevented by following the > relatively simple rfc process and having a more clear way to define > what is acceptable as BC. >
I think, the decision of the majority of voters, told that the BC break is minor and acceptable for 7.1. The only mistake was in shorten discussion period.
> > I think the RMs should step in here. >
Yeah, I suppose, RMs may have a right to take a decision and revert this. Thanks. Dmitry.

Davey

10 years ago
On Wed, Jun 15, 2016 at 11:25 PM, Dmitry Stogov <dmitry@zend.com> wrote:
> Hi Pierre, > > > On 06/16/2016 09:18 AM, Pierre Joye wrote: > >> >> Hi Dmitry >> >> I am sorry but I have to ask to wait before merging it. >> >> > Sorry, but this is already merged. > >> >> It is definitely not clear that: >> >> . The rfc was valid to begin with due to the short discussion time >> . This BC is acceptable for 7.x >> >> I think things like things can be prevented by following the relatively >> simple rfc process and having a more clear way to define what is acceptable >> as BC. >> >> > I think, the decision of the majority of voters, told that the BC break is > minor and acceptable for 7.1. > The only mistake was in shorten discussion period. > >> >> I think the RMs should step in here. >> >> > Yeah, I suppose, RMs may have a right to take a decision and revert this. > > Thanks. Dmitry.
As I said in earlier discussions, the fact we cannot measure potential impact makes me very hesitant to include this change. Again: if I were the sole RM I _would_ try to veto this change. I understand the desire to get this out in 7.1, we will never have a time when 7.x has less users than it does now, and we don't want to wait for 8.0. IMO a better compromise would have been to add an E_DEPRECATED noting that it would be changed in 7.2, and then doing so. Is something like this still a possibility? I know that Joe feels strongly about this particular change being merged in, and it is done now. We should address similar situations moving forward, and move on. - Davey

Matteo Beccati

10 years ago
On 16/06/2016 08:18, Pierre Joye wrote:
> Hi Dmitry > > I am sorry but I have to ask to wait before merging it. > > It is definitely not clear that: > > . The rfc was valid to begin with due to the short discussion time > . This BC is acceptable for 7.x
As Dmitry pointed out the change has been merged. I had voted yes, and I was prepared to make a few adjustments to the legacy app I work with. You can imagine I was extremely surprised to see that the test suite was green this morning. There might still be some fixing required for the code that isn't covered by tests, but to me this BC-break seems almost irrelevant compared to what we had in 7.0 (e.g. PHP4 constructors or method signatures). Of course I can't speak for the majority of the applications out there, but I think Revive Adserver is a fairly good sample of legacy code one could find. Cheers
-- Matteo Beccati Development & Consulting - http://www.beccati.com/

Pierre Joye

10 years ago
On Fri, Jun 17, 2016 at 4:57 PM, Matteo Beccati <php@beccati.com> wrote:
> On 16/06/2016 08:18, Pierre Joye wrote: >> Hi Dmitry >> >> I am sorry but I have to ask to wait before merging it. >> >> It is definitely not clear that: >> >> . The rfc was valid to begin with due to the short discussion time >> . This BC is acceptable for 7.x > > As Dmitry pointed out the change has been merged. I had voted yes, and I > was prepared to make a few adjustments to the legacy app I work with. > > You can imagine I was extremely surprised to see that the test suite was > green this morning. There might still be some fixing required for the > code that isn't covered by tests, but to me this BC-break seems almost > irrelevant compared to what we had in 7.0 (e.g. PHP4 constructors or > method signatures). > > Of course I can't speak for the majority of the applications out there, > but I think Revive Adserver is a fairly good sample of legacy code one > could find.
That change is relatively harmless, right. This is less the case for the other RFC which has been now retargetted for 8. Overall we should be much more careful than simply "it is ok, only a few adjustments". This is exactly what prevented people to upgrade. And now that we finally see a very fast adoption of latest releases, it would be a bad time to go back to always have small breaks per new release. As I feel confident that more will come. Cheers,
-- Pierre @pierrejoye | http://www.libgd.org