coenjacobs / conductor

Enforces your WordPress plugin requirements by checking PHP versions and details of other plugins
5 stars 1 forks source link

Customize messages #1

Open danieliser opened 4 years ago

danieliser commented 4 years ago

@coenjacobs Just found your libs and in effort to simply start relying more on packages than writing our own code, specifically when it comes to boilerplate stuff.

We have a similar setup and I'd like to either find a way to get benefits we have from years of using our own merged into this library where appropriate, or being able to overload them where needed.

The first and biggest is the plugin check message. With our Popup Maker plugin's extension activator it checks for the core:

https://github.com/PopupMaker/Popup-Maker/blob/master/classes/Extension/Activator.php#L162-L180

I am happy to submit a PR with those enhancements.

One possibility would be something similar to another wp requirement package and accept an i18n array as an argument for Messages: https://packagist.org/packages/wearerequired/wp-requirements-check

coenjacobs commented 4 years ago

Sorry for not picking up on this issue before. I was somehow not being notified of it, or it must have gone under the radar.

Customising messages is absolutely something that I'd like to be added to this package. It's been stale for quite a while, but I definitely see the potential still. Accepting an array with the translations is something I'm okay with, I think that is the best possible solution that I can imagine right now. So if you're up for it, a pull request is more than welcome! :)

danieliser commented 4 years ago

@coenjacobs - Awesome will play with it this weekend.

Did you like the idea of allowing it to detect activation/install status and change up he messaging, or would you prefer that stays in our own logic?

That is if the plugin that is required is installed, just not active, show a notice that has a link to activate it,

If its a free wordpress.org plugin and its not installed it could show a link to install it etc.

That would be a separate PR, but just curious if you'd have interest in that type of functionality.

danieliser commented 4 years ago

@coenjacobs - So I've been playing around with this one. Need some feedback on how you prefer the i18n data get injected.

First assumption is that the array of strings should be passed into Messages::setup($checks = [], $i18n = []) or as a general $options array for the second arg that contains an 'i18n' key. The latter of course leaves you open to extend it further at a cost of not being nearly as explicit.

The array of i18n messages can be passed through wp_parse_args pretty easily.

Question #1 Which of the above method signatures do you prefer.

Question #2 Do you prefer we inject the config $i18n into the $check->getMessage( $i18n ) as a new optional paremeter?

Question #3 Assuming Yes to 2 above, do you want to keep default messaging inside each check to keep them self contained, or move those default message declarations Messages::setup method and always pass them in through getMessage() or whatever setup is used for 2 above.

Question #4 If default messages are kept in the Check classes, are you ok with moving them into an array that can then be merged with the custom messages passed in, as opposed to them being inline inside the if/else statements directly. This will just simplify the process of merging in custom messages with defaults.

Likely more questions as I put this together as getting the options all the way down the tree is likely the tricky part here, but its doable, just want to make sure I follow your preferred methodology.