Skip to content

handle class constant visibility added in PHP-7.1 #59

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 3, 2016

Conversation

marc-mabe
Copy link
Owner

PHP-7.1 will support Class Constant Visibility
but should private and/or protected class constants be part of an enumeration?

@marc-mabe marc-mabe self-assigned this Aug 17, 2016
@prolic
Copy link
Collaborator

prolic commented Aug 17, 2016

Hardware question. Maybe make it configurable?

@marc-mabe marc-mabe force-pushed the const_visibility branch 2 times, most recently from d6dea6c to 1b384b2 Compare August 17, 2016 21:47
@prolic
Copy link
Collaborator

prolic commented Aug 18, 2016

After thinking about it, I find that only public consts should be part of the enumeration. At least by default. I don't have a use case at hand, but private or protected consts should only be allowed via a switch enabling this feature in the constructor.

@marc-mabe
Copy link
Owner Author

I also feel that only public constants makes sense as enumeration members.

Having a switch is somewhat problematic as an enumeration should be seen as a type implement with a lot of static member features here.

I need to check how to detect public constants only - need to compile PHP-7.1 locally as it's not fun to develop with travis as environment ;)

Am 18.08.2016 um 10:02 schrieb Sascha-Oliver Prolic notifications@github.com:

After thinking about it, I find that only public consts should be part of the enumeration. At least by default. I don't have a use case at hand, but private or protected consts should only be allowed via a switch enabling this feature in the constructor.


You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

@marc-mabe marc-mabe force-pushed the const_visibility branch 10 times, most recently from 9550ea7 to 431c0e4 Compare August 21, 2016 21:13
@marc-mabe marc-mabe force-pushed the const_visibility branch 4 times, most recently from bb6c6ef to f0156d8 Compare September 18, 2016 20:25
@marc-mabe marc-mabe changed the title [WIP] handle class constant visibility handle class constant visibility added in PHP-7.1 Sep 18, 2016
@marc-mabe
Copy link
Owner Author

works now -> only public constants are used as enumerators

@marc-mabe marc-mabe added this to the 2.3.0 milestone Sep 18, 2016
Copy link
Collaborator

@prolic prolic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goods, just some minor things to clean up

$scopeConstants = array();
if (PHP_VERSION_ID >= 70100) {
// Since PHP-7.1 visibility modifiers are allowed for class constants
// for enumerations we are only interected in public once.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/s/interected/interested

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

protected const PRO2 = 'protected extended';
private const PRI2 = 'private extended';
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one new line too much

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

protected const PRO = 'protected';
private const PRI = 'private';
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one new line too much

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed


public function testConstVisibilityExtended()
{
if (PHP_VERSION_ID < 70100) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better like this:

version_compare(PHP_VERSION, '7.1', '<')


public function testConstVisibility()
{
if (PHP_VERSION_ID < 70100) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better like this:

version_compare(PHP_VERSION, '7.1', '<')


do {
$scopeConstants = array();
if (PHP_VERSION_ID >= 70100) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better like this:

version_compare(PHP_VERSION, '7.1', '<')

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

using version_compare() is much much slower than using the integer constant as it first needs to call a function and the function has to parse three strings into two integers and a comparator sign. Instead I'm using the PHP_VERSION_ID integer constant to just do one comparison of two integers ;)

@marc-mabe
Copy link
Owner Author

@prolic thanks for review - I fixed everything except the use of PHP_VERSION_ID as explained inline ;)

@prolic
Copy link
Collaborator

prolic commented Sep 30, 2016

perfect

@marc-mabe marc-mabe merged commit 6b5f052 into master Oct 3, 2016
@marc-mabe marc-mabe deleted the const_visibility branch October 3, 2016 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants