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

9 years ago by Dmitry Stogov — view source — reply

unread

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.

9 years ago by Pierre Joye — view source — reply

unread

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

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.

9 years ago by Dmitry Stogov — view source — reply

unread

Hi Pierre,

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.

Thanks,
Pierre

On Jun 16, 2016 1:12 PM, "Dmitry Stogov" <dmitry@zend.com
mailto:dmitry@zend.com> wrote:
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.

9 years ago by Davey Shafik — view source — reply

unread

Hi Pierre,

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

9 years ago by Matteo Beccati — view source — reply

unread

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/

9 years ago by Pierre Joye — view source — reply

unread

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