@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.

@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)

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.

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!

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

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;
}

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

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

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;
}

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

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?

@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

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

Please consider adding getDocComment method to ReflectionClassConstant.

@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.

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

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 :)

@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!

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; }
}

@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)

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.

@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 :)

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

@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?

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

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.

@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.

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!

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

thanks

Please review another PR based on this one #1662

close this, focus on #1662 instead