search

WPupdatePHP / wp-update-php

Library to be bundled with WordPress plugins to enforce users to upgrade to supported PHP versions.
GNU General Public License v2.0
79 stars 11 forks source link

pass __FILE__ to constructor to compute #13

Closed afragen closed 9 years ago

afragen commented 9 years ago

plugin name and automatically add plugin name to notice.

afragen commented 9 years ago

Not sure if https://github.com/afragen/wp-update-php/commit/9c96b509e38456aa94e549cc66cfde07423ba276 is how you want to explain how to use this but it seems to be simpler to exit if conditional is false than wrap entire plugin instantiation code in this conditional. This would work in all cases.

afragen commented 9 years ago

@coenjacobs did I do too much here?

coenjacobs commented 9 years ago

Yes. :smile: Please make separate pull requests for each of these issues. You can do so by committing them on a separate branch and file a pull request for each of them.

Just going through your changes one by one:

  1. I'm not a huge fan of passing yet another argument to the constructor for the sake of naming the plugin. #9 will introduce a second parameter already and I feel the name of the plugin can be optional so I'd rather introduce a set method for something like this.
  2. The add exit for class_exists, update readme commit looks good. I'll cherry pick that out of this commit in a bit and put it in the 1.1.0 release branch.
  3. I don't know what effect a text domain will have, as we are not loading translation files for any text domain. I'd like to discuss this some more in #8 before we decide on anything.
  4. The README change with the early return is just a different way to do it. This can of course be done in a lot of different ways, but I assume everyone implementing this library will know how they can handle a boolean returning method. Some people might want to trigger a different action based on true/false return there, so I don't want to make the examples too specific.

Anyway, I really appreciate your contributions and I hope you understand why I'm closing this pull request now. I'm only accepting 2. at this point as I explained above.

coenjacobs commented 9 years ago

Final note: I left out the require_once call in the README change (cherry-picked commit) as that will be different for each setup as well. I might invest some time in explaining the differences, but at this point the README assumes that the library is loaded via Composer at which point you can utilise the autoloader. I'm not saying that this is the holy grail, but I'd like to at least keep the little documentation we currently have in one line.

afragen commented 9 years ago

I'll start over and make much smaller PRs, but injecting __FILE__ in some method is likely the easiest way to cleanly grab the name of the plugin, unless you want to pass the name directly.

Many will likely want to use this class as I do, to inform users that a particular plugin requires a version of PHP that they do not have and therefore the plugin will not be loaded. It is then necessary that the user know which plugin(s) are throwing this warning.

coenjacobs commented 9 years ago

in some method is likely the easiest way to cleanly grab the name of the plugin, unless you want to pass the name directly.

I'm not opposed to that. I think we only need the name really.

afragen commented 9 years ago

It does simplify the coding and decrease the overhead a small amount. I was originally going for more universality.

Do you want a whole new method for injecting this? I could hook that into the does_it_meet_required_version?