Class Constant Visibility Modifiers #1494

Sean-Der · 2015-08-30T06:17:14Z

@reeze created an RFC here and I emailed internals here and didn't get any responses positive/negative.

This is a WIP and has test fails, and the API has not been finalized. Maybe it should be changed to mirror properties more, but this would cause more breakage.

marcioAlmada · 2015-08-30T06:32:54Z

@reeze created an RFC here and I emailed internals here and didn't get any responses positive/negative.

Hi, I'm sorry that your request for comments had no replies. Anyway, you should also add your RFC to this list, some people discover new proposals through it.

Sean-Der · 2015-08-30T06:45:58Z

@marcioAlmada thanks for linking that!

When I hear back from @reeze and hear if he is ok with me adopting/working with him on the RFC I will add it to that page (or I will have to start my own)

reeze · 2015-08-30T07:36:29Z

Hi @Sean-Der Thanks for your work. I didn't finish my patch but start to working on other things. You are welcome to working on the RFC.

Sean-Der · 2015-08-30T18:54:29Z

Thanks for the quick response @reeze are you interested in working with me on it? I totally understand if you are busy/not interested anymore, I just want to make sure I am not stomping on your work/ideas.

thanks!

reeze · 2015-08-31T04:00:17Z

Hi @Sean-Der Go ahead ;-) I will test your patch and try to contribute to your branch once I have time.

c9s · 2015-09-24T07:22:52Z

One question for the RFC: why the syntax used in interface is different in class? could we make it consistent?

I saw the constant defined in interface use protected:

interface ICache {
    protected const PROTECTED = 0;
}

but the constants defined in class are just:

class Foo {
    protected FOO = 0;
    public BAR = 10;
}

staabm · 2015-09-24T07:38:31Z

@c9s your 2nd example is not valid syntax, see https://3v4l.org/ST9Gp

c9s · 2015-09-24T08:18:45Z

@staabm The code was copied from RFC and it's only in RFC. (this one https://wiki.php.net/rfc/class_const_visibility )

c9s · 2015-09-24T08:20:51Z

I think changing the code below (The RFC one):

class Token {
    // Constants default to public
    const PUBLIC_CONST = 0;

        // Constants then also can have a defined visibility
        private PRIVATE_CONST = 0;
        protected PROTECTED_CONST = 0;
        public PUBLIC_CONST_TWO = 0;
}

to this one is better:

class Token {
    // Constants default to public
    const PUBLIC_CONST = 0;

        // Constants then also can have a defined visibility
        private const  PRIVATE_CONST = 0;
        protected const  PROTECTED_CONST = 0;
        public const  PUBLIC_CONST_TWO = 0;
}

HallofFamer · 2015-09-24T09:15:30Z

I dont think its necessary. Constants are supposed to be public anyway, unless you mean read-only fields, which can be private or protected.

c9s · 2015-09-24T11:26:19Z

So, is this going to be merged or not?

One more question:

If a constant is defined as protected constant in one interface, is it available to classes implemented with the interface?

marcioAlmada · 2015-09-24T14:17:53Z

@Sean-Der @reeze

I believe the inconsistent examples in the RFC are just an oversight. It indeed should be:

class { private const  PRIVATE_CONST = 0; } // valid
class { private PRIVATE_CONST = 0; } // invalid!

@c9s

So, is this going to be merged or not?

For major language changes we have a voting phase. You can watch the votings through http://php-rfc-watch.beberlei.de

Sean-Der · 2015-09-24T14:23:52Z

Hey @marcioAlmada @c9s yep you caught a typo! Pretty embarrassing for such a light RFC, thank you 👍

@HallofFamer In PHP I don't think you can have read-only fields (properties?) now. There are a couple of use cases for a readonly member of a class constant, check out the conversation so far about this here

Furgas · 2015-10-08T06:58:30Z

Please consider adding getDocComment method to ReflectionClassConstant.

marcioAlmada · 2015-10-08T13:37:22Z

@Furgas usually RFCs are hard work and authors tend to add reflection API after the discussion or voting phase, when they already know it's worth committing more time to it.

Sean-Der · 2015-10-20T03:15:19Z

Thanks for sticking up for me @marcioAlmada appreciate it, OpenSource is usually less than welcoming.

Thanks @Furgas for being interested in this RFC! getDocComment is now implemented by 47bdc4a

marcioAlmada · 2015-10-20T03:43:08Z

What will be your stance regarding ReflectionClass and it's public API? Currently:

ReflectionClass::getConstants returns an array of constants [name => value]
ReflectionClass::getConstant returns the value of the constant

You're changing this behavior, don't you? The "RFC impact" section must be explicit about it :)

Sean-Der · 2015-10-20T05:31:01Z

@marcioAlmada Yep those should be changed as well!

I will make those changes and update the RFC, have plenty of failing tests also. Trying to hit the bare minimum to just get the RFC approved. Thanks for looking it all over!

thanks!

nikic · 2015-10-21T15:19:37Z

Zend/zend_API.c

+		ce->constants_table = perealloc(ce->constants_table, sizeof(zval) * ce->constants_count, ce->type == ZEND_INTERNAL_CLASS);
+		zend_hash_add_ptr(&ce->constants_info, name, const_info);
+	} else {
+		i_zval_ptr_dtor(&ce->constants_table[const_info->offset] ZEND_FILE_LINE_CC);


Shouldn't this error instead?

The old behavior was to just update the value of the constant if it already existed, and some extensions in tree now depend on it (Date being one of them)

I thought better to keep the existing behavior, because maybe extensions outside php-src depend on this?

Majkl578 · 2015-10-22T01:02:18Z

Hi,
although I like the idea to have private/protected constants, I think the RFC should be rejected in current state, because the changes to Reflection API are a very serious BC break that will break a lot of user-land code. 😟

Also, there seems to be a conflict in trait resolution for constants and methods with same name (based on an example from http://news.php.net/php.internals/88906). What will this code do?

trait TestTrait {
    const foo = 'const';
    function foo() { echo 'method'; }
}

class TestClass {
    use TestTrait { foo as bar; }
}

Sean-Der · 2015-10-22T02:46:30Z

@Majkl578 that example was wrong (it was mine) sorry please ignore the trait stuff.

I am sorry if the Reflection changes cause any issues for you! However, if the change was not made we would be missing out on important information (flags + docComments) It will be a while before this code ends up on production machines though (7.1 is a long time away)

marcioAlmada · 2015-10-22T04:55:30Z

Hi!

@Majkl578

Also, there seems to be a conflict in trait resolution for constants and methods with same name (based on an example from http://news.php.net/php.internals/88906). What will this code do?

First of all: currently, traits cannot have constants so the problem doesn't even exists https://3v4l.org/gCvYH.

Even if we add constants to traits one day (and I hope this day never comes), we could have:

trait TestTrait {
    const foo = 'const';
    function foo() { echo 'method'; }
}

class TestClass {
    use TestTrait {
        const foo as bar;
        foo as bar;
    }
}

I don't see any issue here 😉

Regarding the Reflection API break, @nikic proposed an alternative. Instead of changing the return values of existing methods, we could just include ReflectionClass::getReflectionConstants() and ReflectionClass::getReflectionConstant($constant_name).

Seems reasonable, even though I prefer the BC break.

Sean-Der · 2015-10-22T06:49:57Z

@marcioAlmada I prefer the BC break as well.

My rationale is

getConstant/getConstants is not something that is going to be very useful to applications (Reflection itself always for me has been great for testing/introspection not making stuff happen at runtime)
The fix it easy, just call getValue() and you are done
That is some pretty annoying baggage to take on

But always happy to take to a vote :)

laruence · 2015-10-23T07:30:18Z

I think it's better if you can squash all you changes into one commit... , there are lots of commits now

Sean-Der · 2015-10-23T14:34:55Z

@laruence done!

Are you able to reproduce those test failures on a ZTS build?

My CLI SAPI returns

PHP 7.1.0-dev (cli) (built: Oct 23 2015 00:20:56) ( ZTS DEBUG )
Copyright (c) 1997-2015 The PHP Group 
Zend Engine v3.1.0-dev, Copyright (c) 1998-2015 Zend Technologies

But make test doesn't get any of the same segfaults, and valgrind shows no invalid frees/reads?

nikic · 2015-10-23T14:46:13Z

@Sean-Der The debug build on Travis also enabled opcache. That's likely where the issue is.

Majkl578 · 2015-10-27T00:06:55Z

Thanks for the clarification about traits, that's not an issue for now.

I understand your reasons for breaking compatibility in relfection, but this is an excellent example of a change that becomes a nighmare for projects that need to keep compatibility with multiple versions of PHP.

And since this RFC targets 7.1, it should also be noted that it violates PHP's release process, which prohibits userland API BC breaks in x.y+1.z versions.

Sean-Der · 2015-11-01T19:59:06Z

@Majkl578 That is a great point, what do you think of not breaking the old API at all and doing @marcioAlmada's suggestion?

Adding two new subroutines getReflectionConstant and getReflectionConstants and keeping getConstant and getConstants unchanged.

Sean-Der · 2015-11-02T04:52:30Z

And done!

I implemented @marcioAlmada's suggestion, and no more BC in this commit anymore.

AFAIK this is ready for review (and hopefully merge) if anyone has any suggestions/guidance on better tests or who to bother to get the merge process going would love to hear!

[0] https://wiki.php.net/rfc/class_const_visibility

Sean-Der · 2015-12-03T06:50:45Z

@dstogov Hey! Would you mind reviewing the opcache changes, the entire test suite does pass though.

thanks

dstogov · 2015-12-03T09:18:43Z

Zend/zend.h

 	HashTable function_table;
 	HashTable properties_info;
-	HashTable constants_table;
+	HashTable constants_info;


I think, it's going to be more efficient to introduce zend_class_constant structure and keep such structures in HashTable constants_table. We don't need separate "info" and "table" because, in opposite to properties, constants can't be changed in run-time.

Lucky to get a review out of you! Thanks for taking a careful look at this, and keeping php-src in top shape :)

If there is anything I can do code wise, just tell me. Would love to stay involved, but it is more efficient for you to work alone.

thanks

dstogov · 2015-12-04T23:55:02Z

Please review another PR based on this one #1662

laruence · 2015-12-05T12:49:55Z

close this, focus on #1662 instead

laruence added the RFC label Aug 30, 2015

nikic reviewed Oct 21, 2015
View reviewed changes

Sean-Der closed this Oct 23, 2015

Sean-Der reopened this Oct 23, 2015

Add visibility modifiers to class constants as designed in [0]

fb2bdb6

[0] https://wiki.php.net/rfc/class_const_visibility

dstogov reviewed Dec 3, 2015
View reviewed changes

laruence closed this Dec 5, 2015

dstogov mentioned this pull request Dec 5, 2015

Refactored immplementation of https://wiki.php.net/rfc/class_const_vi… #1662

Closed

Sean-Der deleted the bug-69980-class-constants branch January 2, 2016 05:26

Class Constant Visibility Modifiers #1494

Class Constant Visibility Modifiers #1494

Uh oh!

Conversation

Sean-Der commented Aug 30, 2015

Uh oh!

marcioAlmada commented Aug 30, 2015

Uh oh!

Sean-Der commented Aug 30, 2015

Uh oh!

reeze commented Aug 30, 2015

Uh oh!

Sean-Der commented Aug 30, 2015

Uh oh!

reeze commented Aug 31, 2015

Uh oh!

c9s commented Sep 24, 2015

Uh oh!

staabm commented Sep 24, 2015

Uh oh!

c9s commented Sep 24, 2015

Uh oh!

c9s commented Sep 24, 2015

Uh oh!

HallofFamer commented Sep 24, 2015

Uh oh!

c9s commented Sep 24, 2015

Uh oh!

marcioAlmada commented Sep 24, 2015

Uh oh!

Sean-Der commented Sep 24, 2015

Uh oh!

Furgas commented Oct 8, 2015

Uh oh!

marcioAlmada commented Oct 8, 2015

Uh oh!

Sean-Der commented Oct 20, 2015

Uh oh!

marcioAlmada commented Oct 20, 2015

Uh oh!

Sean-Der commented Oct 20, 2015

Uh oh!

nikic Oct 21, 2015

Choose a reason for hiding this comment

Uh oh!

Sean-Der Oct 21, 2015

Choose a reason for hiding this comment

Uh oh!

Majkl578 commented Oct 22, 2015

Uh oh!

Sean-Der commented Oct 22, 2015

Uh oh!

marcioAlmada commented Oct 22, 2015

Uh oh!

Sean-Der commented Oct 22, 2015

Uh oh!

laruence commented Oct 23, 2015

Uh oh!

Sean-Der commented Oct 23, 2015

Uh oh!

nikic commented Oct 23, 2015

Uh oh!

Majkl578 commented Oct 27, 2015

Uh oh!

Sean-Der commented Nov 1, 2015

Uh oh!

Sean-Der commented Nov 2, 2015

Uh oh!

Sean-Der commented Dec 3, 2015

Uh oh!

dstogov Dec 3, 2015

Choose a reason for hiding this comment

Uh oh!

Sean-Der Dec 3, 2015

Choose a reason for hiding this comment

Uh oh!

dstogov commented Dec 4, 2015

Uh oh!

laruence commented Dec 5, 2015

Uh oh!