eloquent / enumeration

An enumeration implementation for PHP.
MIT License
147 stars 8 forks source link

Added support for non-public enumeration constants #26

Closed Bilge closed 5 years ago

Bilge commented 5 years ago

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[bot] commented 5 years ago

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master    #26   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           6      6           
  Lines         175    175           
=====================================
  Hits          175    175
Impacted Files Coverage Δ
src/AbstractEnumeration.php 100% <100%> (ø) :arrow_up:
src/AbstractMultiton.php 100% <0%> (ø) :arrow_up:

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.

ezzatron commented 5 years ago

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 commented 5 years ago

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

ezzatron commented 5 years ago

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

Bilge commented 5 years ago

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

ezzatron commented 5 years ago

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 commented 4 years ago

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 commented 4 years ago

@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 commented 4 years ago

Brilliant - thanks!