Everything

php

[RFC] [VOTE] Class Constant Visibility

php.internals

Sean DuBois

10 years ago

Hey list,

Time for a simple RFC (in theory)!

I would like to add visibility modifiers to class constants, and then
as a nice added bonus give more info from the Reflection API (constants
now are a dedicated class so nice things like doc comments)

https://wiki.php.net/rfc/class_const_visibility

thanks!

Dan Ackroyd

10 years ago

Hi Sean, internals,

While I support this RFC, it seems that the actual implementation is
still under discussion.

In particular, whether the RFC is going to allow protected constants
in an interface is not clear.

In the text of the RFC, on one line it says: "This RFC propose PHP
support class constant visibility that mirror the behaviour of method
and property visibility." This would imply that protected constants
are not allowed, as protected methods are not allowed in interfaces.

Just below it says: "//Interfaces only support protected/public const,
and a compile time error will be throw for privates" Which says
clearly that protected constants _would_ be allowed.

Please can you update the text of the RFC?

Unfortunately, that may need a restart of the voting :-\ However that
should just be a formality as it seems to be a popular RFC.

cheers
Dan

Sean DuBois

10 years ago

On Wed, Oct 21, 2015 at 03:54:48PM +0000, Dan Ackroyd wrote:

> Hi Sean, internals,
>
> While I support this RFC, it seems that the actual implementation is
> still under discussion.
>
> In particular, whether the RFC is going to allow protected constants
> in an interface is not clear.
>
> In the text of the RFC, on one line it says: "This RFC propose PHP
> support class constant visibility that mirror the behaviour of method
> and property visibility." This would imply that protected constants
> are not allowed, as protected methods are not allowed in interfaces.
>
> Just below it says: "//Interfaces only support protected/public const,
> and a compile time error will be throw for privates" Which says
> clearly that protected constants _would_ be allowed.
>
> Please can you update the text of the RFC?
>
> Unfortunately, that may need a restart of the voting :-\ However that
> should just be a formality as it seems to be a popular RFC.
>
> cheers
> Dan
>
> --

Hi!

That is an unfortunate mistake on my part. The *proper* way to adjust
visibility of a trait from a class constant is the following.

trait myTrait {
public const FOO = 'bar'
}

class myClass {
use myTrait { FOO as protected; }
}

class myClass {
use myTrait { FOO as private; }
}

We should not allow anything besides public in trait, this mirrors the design of methods.

If this is right (mind just sending an ACK to make sure I got it right this time) I
will update the RFC. Who decides if/when we should restart voting?

thanks

Levi Morrison

10 years ago

> Who decides if/when we should restart voting?

I don't believe we've ever formalized that aspect. I know in the past
sometimes RFC authors will notice something wrong and fix it, then
restart voting themselves.

Aaron Piotrowski

10 years ago

Hi Sean,

> On Oct 21, 2015, at 11:17 AM, Sean DuBois <sean@siobud.com> wrote:
>
> On Wed, Oct 21, 2015 at 03:54:48PM +0000, Dan Ackroyd wrote:
>> Hi Sean, internals,
>>
>> While I support this RFC, it seems that the actual implementation is
>> still under discussion.
>>
>> In particular, whether the RFC is going to allow protected constants
>> in an interface is not clear.
>>
>> In the text of the RFC, on one line it says: "This RFC propose PHP
>> support class constant visibility that mirror the behaviour of method
>> and property visibility." This would imply that protected constants
>> are not allowed, as protected methods are not allowed in interfaces.
>>
>> Just below it says: "//Interfaces only support protected/public const,
>> and a compile time error will be throw for privates" Which says
>> clearly that protected constants _would_ be allowed.
>>
>> Please can you update the text of the RFC?
>>
>> Unfortunately, that may need a restart of the voting :-\ However that
>> should just be a formality as it seems to be a popular RFC.
>>
>> cheers
>> Dan
>>
>> --
> Hi!
>
> That is an unfortunate mistake on my part. The *proper* way to adjust
> visibility of a trait from a class constant is the following.
>
>    trait myTrait {
>        public const FOO = 'bar'
>    }
>
>    class myClass {
>        use myTrait { FOO as protected; }
>    }
>
>    class myClass {
>        use myTrait { FOO as private; }
>    }
>
> We should not allow anything besides public in trait, this mirrors the design of methods.
>
> If this is right (mind just sending an ACK to make sure I got it right this time) I
> will update the RFC. Who decides if/when we should restart voting?
>
> thanks
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>

I think you may have misunderstood. Traits should be able to have protected and private constants. The issue that Dan pointed out is with an interface having a protected constant in the example code, contradicting what was written prior in the RFC about mirroring the current behavior of method and property visibility.

Interfaces should only be allowed to have public constants. Protected constants would infer that the interface knows something about implementation, which doesn’t make sense for an interface. I think the RFC should be updated to remove this ambiguity.

I’m not sure if this would require the vote to be restarted, as I doubt anyone who voted yes would now vote no with this change.

Regards,
Aaron Piotrowski

Sean DuBois

10 years ago

On Wed, Oct 21, 2015 at 12:31:58PM -0500, Aaron Piotrowski wrote:

> Hi Sean,
>
> > On Oct 21, 2015, at 11:17 AM, Sean DuBois <sean@siobud.com> wrote:
> >
> > On Wed, Oct 21, 2015 at 03:54:48PM +0000, Dan Ackroyd wrote:
> >> Hi Sean, internals,
> >>
> >> While I support this RFC, it seems that the actual implementation is
> >> still under discussion.
> >>
> >> In particular, whether the RFC is going to allow protected constants
> >> in an interface is not clear.
> >>
> >> In the text of the RFC, on one line it says: "This RFC propose PHP
> >> support class constant visibility that mirror the behaviour of method
> >> and property visibility." This would imply that protected constants
> >> are not allowed, as protected methods are not allowed in interfaces.
> >>
> >> Just below it says: "//Interfaces only support protected/public const,
> >> and a compile time error will be throw for privates" Which says
> >> clearly that protected constants _would_ be allowed.
> >>
> >> Please can you update the text of the RFC?
> >>
> >> Unfortunately, that may need a restart of the voting :-\ However that
> >> should just be a formality as it seems to be a popular RFC.
> >>
> >> cheers
> >> Dan
> >>
> >> --
> > Hi!
> >
> > That is an unfortunate mistake on my part. The *proper* way to adjust
> > visibility of a trait from a class constant is the following.
> >
> >    trait myTrait {
> >        public const FOO = 'bar'
> >    }
> >
> >    class myClass {
> >        use myTrait { FOO as protected; }
> >    }
> >
> >    class myClass {
> >        use myTrait { FOO as private; }
> >    }
> >
> > We should not allow anything besides public in trait, this mirrors the design of methods.
> >
> > If this is right (mind just sending an ACK to make sure I got it right this time) I
> > will update the RFC. Who decides if/when we should restart voting?
> >
> > thanks
> >
> > --
> > PHP Internals - PHP Runtime Development Mailing List
> > To unsubscribe, visit: http://www.php.net/unsub.php
> >
>
> I think you may have misunderstood. Traits should be able to have protected and private constants. The issue that Dan pointed out is with an interface having a protected constant in the example code, contradicting what was written prior in the RFC about mirroring the current behavior of method and property visibility.
>
> Interfaces should only be allowed to have public constants. Protected constants would infer that the interface knows something about implementation, which doesn’t make sense for an interface. I think the RFC should be updated to remove this ambiguity.
>
> I’m not sure if this would require the vote to be restarted, as I doubt anyone who voted yes would now vote no with this change.
>
> Regards,
> Aaron Piotrowski
>
>

Argh I am rushing, again my mistake.

I just updated the RFC to reflect this, sorry about the confusion
everyone! The intent of the RFC is to just copy existing patterns from
methods/properties so as non-controversial as possible.

For now I am not going to restart the vote, if anyone feels
this decision is wrong I am more than happy to restart the vote.
However, don't want to inconvenience the people who have already taken
time to vote.

thanks!

Dan Ackroyd

10 years ago

On 21 October 2015 at 16:17, Sean DuBois <sean@siobud.com> wrote:

>
>  I will update the RFC. Who decides if/when we should restart voting?

"The PHP RFC rules is more what you'd call "guidelines" than actual
rules" as there are no set decision makers or rules enforcers.

The rules set down for this scenario are, in theory, pretty clear:

> Based on the result of the votes and the discussion there are three possible outcomes:
>
> A serious issue with your RFC needs to be addressed: update the status of your RFC
> page and its section on https://wiki.php.net/RFC to “Under Discussion” and continue
> again from step 5.

But in practice what is a 'serious issue' is not defined.

I think it is almost always right to restart a vote for anything other
than the most minor change as:

i) It avoids setting a precedent where an RFC is changed substantially
while under vote.

ii) It avoids anyone being able to complain that either what was
implemented was not what they voted for, or for people to be able to
ask that the 'missing' behaviour be implemented as a bug-fix.

And yes, in this case the RFC restarting the vote could be considered overkill.

I can't see anyone actually changing their mind due to the text being
clarified, and so the result of the vote will be overwhelming support,
again. But following the rules scrupulously when the outcome is clear,
makes it harder for people to bend the rules when the outcome isn't as
clear.

cheers
Dan

Pascal MARTIN

10 years ago

Le 20/10/2015 19:36, Sean DuBois a écrit :

> Time for a simple RFC (in theory)!
>
> https://wiki.php.net/rfc/class_const_visibility
>
> thanks!

Hi,

After discussing this RFC with other people at AFUP, we are +1.

Basically, being able to define constants inside a class, to represent
magic-values or the like without exposing them publicly, would be nice!
And a bit more consistency with properties and methods cannot hurt.

Thanks for your work on this!

--
Pascal MARTIN, AFUP - French UG
http://php-internals.afup.org/

Dmitry Stogov

10 years ago

Hi Sean,

Although the RFC is accepted, the patch has to be significantly reworked.
Probably, it's going to be better and faster if I do it myself.
Let me know, if you are agree (I'll do this when I have time, and show you
PR before commit).

Thanks. Dmitry.

On Tue, Oct 20, 2015 at 8:36 PM, Sean DuBois <sean@siobud.com> wrote:

Sean DuBois

10 years ago

Hi Dmitry!
Thanks for the quickly reply.

That is a bummer that the patch isn't gonna work! But yes, please go
ahead and take it on! You don't have to show me the PR (I am happy with
what ever you choose to do). There are some
tests so once every passes I will be excited to see it merge :)

On Thu, Dec 03, 2015 at 12:29:50PM +0300, Dmitry Stogov wrote:

Dmitry Stogov

10 years ago

OK. thanks. I'll try to find time to do this on next week.
In any case I'll give a chance to review the future patch.
Somebody might get other good improvement ideas and we don't have to hurry
now :)

Thanks. Dmitry.

On Thu, Dec 3, 2015 at 6:46 PM, Sean DuBois <sean@siobud.com> wrote:

Dmitry Stogov

10 years ago

I've reworked your patch https://github.com/php/php-src/pull/1662
Actually, you patch was really good. I just re-factored one base
data-structure "mistake" and fixed another obvious mistake in reflection.
Anyway, this needs to be re-viewed by others. I'll do it myself on next
week :)

Thanks. Dmitry.


On Thu, Dec 3, 2015 at 7:09 PM, Dmitry Stogov <dmitry@zend.com> wrote:

Dmitry Stogov

10 years ago

Hi Nikita,

Thanks for code review.
All the reported issues should be fixed now.

Thanks. Dmitry.

On Sat, Dec 5, 2015 at 3:02 AM, Dmitry Stogov <dmitry@zend.com> wrote:

Dmitry Stogov

10 years ago

Hi Sean,

The PR has been merged into master.
Please update the RFC accordingly.
Thanks to Nikita and Xinchen for code review and suggestions.

Thanks. Dmitry.

On Mon, Dec 7, 2015 at 11:38 AM, Dmitry Stogov <dmitry@zend.com> wrote:

Michael Wallner

10 years ago

On 08/12/15 10:56, Dmitry Stogov wrote:

> Hi Sean,
>
> The PR has been merged into master.
> Please update the RFC accordingly.
> Thanks to Nikita and Xinchen for code review and suggestions.
>

This patch seems to cause problems for me with registering constants on
internal interfaces; see bug #71413 https://bugs.php.net/71413

--
Regards,
Mike

Xinchen Hui

10 years ago

Hey:

On Wed, Jan 20, 2016 at 7:05 PM, Michael Wallner <mike@php.net> wrote:

> On 08/12/15 10:56, Dmitry Stogov wrote:
> > Hi Sean,
> >
> > The PR has been merged into master.
> > Please update the RFC accordingly.
> > Thanks to Nikita and Xinchen for code review and suggestions.
> >
>
>
> This patch seems to cause problems for me with registering constants on
> internal interfaces; see bug #71413 https://bugs.php.net/71413

this should be fixed in :
https://github.com/php/php-src/commit/62c1c11ad34103729988df9edea343337a900ba9

thanks

>
>
>
> --
> Regards,
> Mike
>
> --
> PHP Internals - PHP Runtime Development Mailing List
> To unsubscribe, visit: http://www.php.net/unsub.php
>

--
Xinchen Hui
@Laruence
http://www.laruence.com/

Michael Wallner

10 years ago

On 21/01/16 06:36, Xinchen Hui wrote:

> Hey:
>
> On Wed, Jan 20, 2016 at 7:05 PM, Michael Wallner <mike@php.net
> <mailto:mike@php.net>> wrote:
>
>     On 08/12/15 10:56, Dmitry Stogov wrote:
>     > Hi Sean,
>     >
>     > The PR has been merged into master.
>     > Please update the RFC accordingly.
>     > Thanks to Nikita and Xinchen for code review and suggestions.
>     >
>
>
>     This patch seems to cause problems for me with registering constants on
>     internal interfaces; see bug #71413 https://bugs.php.net/71413
>
> this should be fixed in :
> https://github.com/php/php-src/commit/62c1c11ad34103729988df9edea343337a900ba9

Confirmed. Thank you!

--
Regards,
Mike