farinspace / wpalchemy

Thin framework for wordpress
http://wpalchemy.com/
Other
416 stars 110 forks source link

Improve PHP version compatibility in line with WordPress minimum requirements #76

Closed aendra-rininsland closed 8 years ago

aendra-rininsland commented 11 years ago

As mentioned in https://github.com/farinspace/wpalchemy/issues/74, the codebase is backwards compatible to PHP4, even though the minimum version currently required for WordPress is 5.2.4. Upgrading the codebase in favour of the more modern syntax makes WPAlchemy less likely to throw depreciated notices in future versions of PHP, in addition to improving documentation engine output. It would also have the added benefit of properly scoping the methods and attributes as intended by the phpDoc comments.

Given that the the WPAlchemy_MetaBox class' phpDoc comments already contain scope attributes, this would be little more difficult than changing the scoping keywords in the classes and ensuring the examples still run without errors.

aendra-rininsland commented 11 years ago

I'm taking a stab at this today. As I do so, however, I'm immediately starting to wonder whether there's any reason wpalchemy/MetaBox.php has so many constants defined at the very beginning.

While I initially intended to do no more than change "var" to the relevant PHP5 class keywords, I'm thinking changing all those defines to either public class properties or class constants would be better long-term — MetaBox.php could be included multiple times (If, for instance, multiple plugins bundled it) without throwing exceptions about redefining constants. This would also greatly simplify the installation by removing the need to put everything in the non-standard wp-content/wpalchemy directory — plugin/theme developers could just include the class with their theme, and reference, for example, WPAlchemy::WPALCHEMY_MODE_ARRAY instead of the current "WPALCHEMY_MODE_ARRAY" constant.

Any thoughts, @farinspace? It would mean a semi-significant API change, and updating the documentation accordingly. Regardless, I'm creating a branch on my fork, "php5-dev", which will contain the other less-drastic changes.

farinspace commented 11 years ago

I am of the same thinking, with a major version there will be a separation between the the old and the new. One of the main things I want to do for this library, just so others are able to contribute with easy is to write some unit tests. I think moving forwards in steps is best, fast releases working towards a better codebase. Thanks again.

aendra-rininsland commented 11 years ago

Ha, rad — we're totally on the same page, I was totally thinking "Y'know, some unit tests would be really helpful right about now..." when I was updating the "var" keywords.

What would you use for unit testing? PHPUnit as per WordPress community norm? Will open a new issue to discuss the various tests that should be written.

farinspace commented 11 years ago

There is a basic test suite I've started a while back, using phpunit and selenium .. see the tests branch .. I've opened an issue for this #79

aendra-rininsland commented 11 years ago

Kinda fell off on this, though I have a bunch of time now and will see what I can add in terms of tests.

One note re: my point in about changing the constants to class constants -- if we do this and get WP-Alchemy refactored to use PHP best practices, not only will it prevent namespace collisions, but it'll be a really quick jump to submitting it to Composer's Packagist directory. This would mean WP-Alchemy can be managed as a dependency using Composer. Pretty much all my recent PHP plugins have used Composer, and it's very much the direction the language is going (IIRC, Drupal is going full Composer for D8).

http://www.getcomposer.org if unfamiliar, it's the current best way of managing PHP deps IMO.

farinspace commented 8 years ago

going through all the issues .. marking most as old/closed, I will likely leave current code on a 1.x branch and only do updates/fixes .. perhaps starting a 2.x branch with best practices