Added support for non-public enumeration constants #26

Bilge · 2018-11-17T23:27:58Z

Previously, non-public constants were being included in enumerations.
However, non-public constants cannot be accessed externally so they
should not be part of the enumeration. This patch adds only public
constants, whether declared explicitly or implicitly, to the
enumeration.

codecov · 2018-11-17T23:29:54Z

Codecov Report

Merging #26 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #26   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         175    175           
=====================================
  Hits          175    175

Impacted Files	Coverage Δ
src/AbstractEnumeration.php	`100% <100%> (ø)`	⬆️
src/AbstractMultiton.php	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9a5bb77...bec2991. Read the comment docs.

Previously, non-public constants were being included in enumerations. However, non-public constants cannot be accessed externally so they should not be part of the enumeration. This patch adds only public constants, whether declared explicitly or implicitly, to the enumeration.

ezzatron · 2018-11-20T01:05:30Z

Thanks for the PR! Looks good.

I'm wondering though, given that getReflectionConstants was introduced in 7.1, and 7.0 security support ends in 12 days from now, I think it might be a better approach to just drop 7.0 support, and implement this feature without needing to check whether the reflection API is available.

The implementation could probably be simplified to the point that it doesn't even need a new method, just use getReflectionConstants instead of getConstants, and check isPublic inside the main loop before creating each instance.

Bilge · 2018-11-20T10:45:28Z

.travis.yml


 before_install:
    - phpenv config-rm xdebug.ini || true
-    - composer config --global github-oauth.github.com $GITHUB_TOKEN


For whatever reason, this configuration setting is not available in the PHP 7.3 build. It should be removed anyway, however, since the invalid token was causing Composer to choke every time it tried to check out a package, stalling the build for longer than necessary.

Not sure what this was doing here in the first place, considering one does not need a token to download public packages.

It used to be the case that Composer would hit GitHub API rate limits when running under Travis CI because of the fact that GitHub counted every request coming from every composer install under Travis CI against the same rate limit quota. The workaround was to supply a personal GitHub token.

In 2016 a change was made to GitHub's infrastructure to remove the rate limit, so it's no longer necessary: composer/composer#4884 (comment)

I think you can probably get rid of the secure environment variable in .travis.yml now too (it contains GITHUB_TOKEN).

Bilge · 2018-11-20T10:46:36Z

As a bonus, I added support for PHP 7.3 since that is due for release soon.

ezzatron · 2018-11-21T04:14:22Z

.travis.yml

-        - php: 7.0
        - php: 7.1
        - php: 7.2
+        - php: 7.3


ezzatron · 2018-11-21T04:14:48Z

.travis.yml


 before_install:
    - phpenv config-rm xdebug.ini || true
-    - composer config --global github-oauth.github.com $GITHUB_TOKEN


It used to be the case that Composer would hit GitHub API rate limits when running under Travis CI because of the fact that GitHub counted every request coming from every composer install under Travis CI against the same rate limit quota. The workaround was to supply a personal GitHub token.

In 2016 a change was made to GitHub's infrastructure to remove the rate limit, so it's no longer necessary: composer/composer#4884 (comment)

I think you can probably get rid of the secure environment variable in .travis.yml now too (it contains GITHUB_TOKEN).

ezzatron · 2018-11-21T04:17:35Z

composer.json

-    },
-    "config": {
-        "platform": {
-            "php": "5.3.99"


I'd prefer to update this instead of deleting it. The new value should be 7.1.99.

Having it here ensures that even if I run composer update under PHP 7.3, the lock file will only ever store dependencies that are at least compatible with PHP 7.1, which is important for running the 7.1 tests.

Libraries shouldn't have a lock file to begin with. I didn't remove the lock file because I'm trying to keep this PR as focused as possible, but if you insist, I will put this back in.

I'd appreciate it. I'm aware of the arguments against committing lock files, but it's not something I want to change in this PR, if at all.

OK, it's done.

ezzatron · 2018-11-21T04:21:29Z

src/AbstractEnumeration.php

    {
        $reflector = new ReflectionClass(get_called_class());

        foreach ($reflector->getConstants() as $key => $value) {


I haven't tested this, but I think we can make this better by using getReflectionConstants directly:

Suggested change

foreach ($reflector->getConstants() as $key => $value) {

foreach ($reflector->getReflectionConstants() as $constant) {

ezzatron · 2018-11-21T04:23:21Z

src/AbstractEnumeration.php


        foreach ($reflector->getConstants() as $key => $value) {
-            new static($key, $value);
+            if ($reflector->getReflectionConstant($key)->isPublic()) {


Suggested change

if ($reflector->getReflectionConstant($key)->isPublic()) {

if ($constant->isPublic()) {

ezzatron · 2018-11-21T04:23:37Z

src/AbstractEnumeration.php

        foreach ($reflector->getConstants() as $key => $value) {
-            new static($key, $value);
+            if ($reflector->getReflectionConstant($key)->isPublic()) {
+                new static($key, $value);


Suggested change

new static($key, $value);

new static($constant->getName(), $constant->getValue());

ezzatron · 2018-11-22T02:52:05Z

Thanks for your work on this PR. 6.0.0 should be available shortly with this feature.

Bilge · 2018-11-22T09:26:35Z

The thought occurs that you might not need to increment the major version since it should be compatible with the 5.x series.

ezzatron · 2018-11-23T05:25:03Z

I avoid doing that because a simple version constraint like ^5 can end up resolving to different minor versions depending on the PHP version used to do the resolution. It's bitten me before, and it can be quite confusing.

joel-pi · 2019-12-12T12:12:05Z

I was using private consts for my enums to prevent people from accidently using them directly instead of by the method. This seems to have stopped me being able to do that.

ezzatron · 2019-12-12T13:38:33Z

@joel-pi You can accomplish the same effect by using AbstractValueMultiton instead of AbstractEnumeration:

use Eloquent\Enumeration\AbstractValueMultiton;

final class ExampleValueMultiton extends AbstractValueMultiton
{
    protected static function initializeMembers()
    {
        new static('FOO', 1);
        new static('BAR', 2);
    }
}

Or if you don't need values, use AbstractMultiton:

use Eloquent\Enumeration\AbstractMultiton;

final class ExampleMultiton extends AbstractMultiton
{
    protected static function initializeMembers()
    {
        new static('FOO');
        new static('BAR');
    }
}

joel-pi · 2019-12-12T17:31:18Z

Brilliant - thanks!

Bilge mentioned this pull request Nov 17, 2018

Non-public constants should not form part of the enumeration #24

Closed

Bilge force-pushed the non-public-constants branch from d766df9 to e44bec1 Compare November 17, 2018 23:33

Bilge force-pushed the non-public-constants branch from 91149f6 to 116bad9 Compare November 20, 2018 10:19

Removed support for PHP < 7.1.

e7ece9c

Bilge force-pushed the non-public-constants branch from 116bad9 to e7ece9c Compare November 20, 2018 10:40

Bilge commented Nov 20, 2018

View reviewed changes

ezzatron suggested changes Nov 21, 2018

View reviewed changes

Bilge added 2 commits November 21, 2018 07:59

Changed constant loop in AbstractEnumeration based on review feedback.

a790c12

Changed Composer config.platform PHP version.

bec2991

ezzatron approved these changes Nov 22, 2018

View reviewed changes

ezzatron merged commit 82130d8 into eloquent:master Nov 22, 2018

ezzatron added this to the Next release milestone Nov 22, 2018

	foreach ($reflector->getConstants() as $key => $value) {
	foreach ($reflector->getReflectionConstants() as $constant) {

	if ($reflector->getReflectionConstant($key)->isPublic()) {
	if ($constant->isPublic()) {

	new static($key, $value);
	new static($constant->getName(), $constant->getValue());

Added support for non-public enumeration constants #26

Added support for non-public enumeration constants #26

Uh oh!

Conversation

Bilge commented Nov 17, 2018

Uh oh!

codecov bot commented Nov 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ezzatron commented Nov 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Bilge commented Nov 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ezzatron commented Nov 22, 2018

Uh oh!

Bilge commented Nov 22, 2018

Uh oh!

ezzatron commented Nov 23, 2018

Uh oh!

joel-pi commented Dec 12, 2019

Uh oh!

ezzatron commented Dec 12, 2019

Uh oh!

joel-pi commented Dec 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Nov 17, 2018 •

edited

Loading