Non-public constants should not form part of the enumeration

[Bilge]

Any constants the enumeration defines with a visibility less than public cannot be accessed externally and thus should not form part of the object's publicly enumerable series; such constants should be regarded as for internal use only.

[Bilge]

@ezzatron If I PR'd a fix for this would you merge it or is this project dead?

[ezzatron]

TL;DR Yes, PR is welcome.

Sorry, I must have missed the initial issue submission. The project isn't dead, but given that I haven't yet removed support for older PHP versions, you might hit some roadblocks trying to make a PR. The repo is probably in need of some maintenance too, so PR tests will most likely fail - not a major issue.

There's the question of whether I get rid of PHP 5 support before or after merging such a PR. I should probably do it at some point anyway. When I do so, I will most likely update the code style / linting which could cause merge conflicts for your PR. Thoughts?

I would probably stop short of removing support for PHP 7.0, which means the PR will have to work for versions of PHP without ReflectionClass::getReflectionConstants. It would need to be used conditionally, preferably based on detection of that method's existence rather than a PHP version check.

[Bilge]

I would probably stop short of removing support for PHP 7.0, which means the PR will have to work for versions of PHP without ReflectionClass::getReflectionConstants.

Being as that's the case there doesn't seem to be any particular need to drop any current versions support. You may wish to pursue that anyway, for other reasons, but as long as we're conditionally supporting PHP 7 reflection features we should assume older versions would continue to work as before.

[ezzatron]

If you're happy to do that, then that's fine by me :) I'll see if I can at least get the Travis build working for all targeted versions so that your PR tests can pass.

[ezzatron]

I just noticed this project is still using git-flow. Give me a sec to rip that out before you branch off.

[Bilge]

@ezzatron I don't think PHP or Travis has changed all that much so I might expect things to still be passing, but I guess we'll see. In any case, I think you can drop HHVM support since most other major open source projects have already done so, add PHP 7.1 and 7.2 to the build matrix and remove 7.0 from allowed failures.

[ezzatron]

Okay, repo maintenance is done. Travis and Codecov should work fine now. This project was more out of date than I expected. There's now a Makefile with:

• make test
• make coverage
• make coverage-open
• make lint

which should come in handy. I've configured the project to publish coverage reports for PHP 7.1, so you can use php-code-coverage annotations to ignore any code branches that are solely for supporting older PHP versions in the coverage report.

Let me know if you need anything else.

[Bilge]

Thanks @ezzatron I appreciate it! However, I have to be honest, it's not high on my priority list at the moment. Nevertheless, I still use this library in almost all my projects so I do expect to get around to it at some point. Meanwhile whoever else works on this will benefit from your upgrades.

Aside, I'm a little surprised you removed Xdebug. Normally it's needed for code coverage generation. Also, I find caching vendor instead of $HOME/.composer is much more effective at reducing build times.

[Bilge]

Also, did you set up a daily cron for this project? I noticed the last build was about two years ago. It may be useful to set up a regularly scheduled build to get notifications in case anything breaks even when our code is not being changed.

[ezzatron]

Aside, I'm a little surprised you removed Xdebug. Normally it's needed for code coverage generation.

I've switched to using phpdbg for code coverage - it's significantly faster, supported by all the coverage tools I know of, and bundled with PHP as of 5.6. The secret sauce is phpdbg -qrr (not a typo), which is how you run scripts in the same fashion as the regular php executable.

As a bonus, everything also runs faster without Xdebug loaded. Composer especially, although it seems like Composer automatically disables Xdebug these days.

Also, I find caching vendor instead of $HOME/.composer is much more effective at reducing build times.

I can't remember off the top of my head, but I think this can cause issues when running lots of different PHP version builds. I'll keep it in mind though, thanks.

Also, did you set up a daily cron for this project?

Yes, I did add a cron build of master, but weekly, which should be sufficient I think.

[Bilge]

@ezzatron Sorry it took a year! The actual work only took me an hour, and is awaiting your review in #26 :slightly_smiling_face:

[ezzatron]

Oh wow, I just re-read this discussion, and realised it was me asking for PHP 7.0 support in the first place 😅

Well, times have changed I guess. I think the code will be much better for having dropped 7.0 support.

[Bilge]

OK l0l. I removed it.

[ezzatron]

Fixed by #26.