10up / generator-wp-make

A Yeoman generator for making WordPress things
184 stars 42 forks source link

Remove `$debug` parameter from `wp_enqueue_scripts` hook function #54

Closed sudar closed 8 years ago

sudar commented 8 years ago

Relevant code

function scripts( $debug = false ) {

add_action( 'wp_enqueue_scripts', $n( 'scripts' ) );

wp_enqueue_scripts hook doesn't pass any parameters to the hooked function.

We should remove the $debug = false parameter, unless there is a specific reason why this is present.

Props @ayebare for finding the issue

lkwdwrd commented 8 years ago

Testability because constants are constant :)

On Friday, December 11, 2015, Sudar Muthu notifications@github.com wrote:

Relevant code https://github.com/10up/generator-wp-make/blob/f3524345692897eeba79d8af513b069916b271df/shared/theme/_core.php#L51

function scripts( $debug = false ) {

add_action( 'wp_enqueue_scripts', $n( 'scripts' ) );

wp_enqueue_scripts hook doesn't pass any parameters to the hooked function.

We should remove the $debug = false parameter, unless there is a specific reason why this is present.

— Reply to this email directly or view it on GitHub https://github.com/10up/generator-wp-make/issues/54.

Luke Woodward – Engineering Manager 10up Inc. http://10up.com | luke.woodward@10up.com | @lkwdwrd

sudar commented 8 years ago

@lkwdwrd The problem is that I have seen lot of cases where the code generated by the generator is copy pasted and used with other hooks like admin_enqueue_scripts that in-fact pass a parameter which results in un-minified files being used always.

lkwdwrd commented 8 years ago

Happy to explore other options, but both cases need to be testable. Maybe a filter would work. I can see your point, but we need another answer than just pulling it out.

On Friday, December 11, 2015, Sudar Muthu notifications@github.com wrote:

@lkwdwrd https://github.com/lkwdwrd The problem is that I have seen lot of cases where the code generated by the generator is copy pasted and used with other hooks like admin_enqueue_scripts that in-fact pass a parameter which results in un-minified files being used always.

— Reply to this email directly or view it on GitHub https://github.com/10up/generator-wp-make/issues/54#issuecomment-164012983 .

Luke Woodward – Engineering Manager 10up Inc. http://10up.com | luke.woodward@10up.com | @lkwdwrd

bswatson commented 8 years ago

@lkwdwrd @sudar

We could preempt the incorrect usage by including an action and a function that has the correct usage applied.

function admin_scripts( $hook_suffix = '', $debug = false ) {}

This would eliminate the ambiguity and hopefully keep others from applying the wp_enqueue_scripts usage to the admin_enqueue_scripts hook.

lkwdwrd commented 8 years ago

I've never been a big fan of the param there. I'm thinking we filter it as an alternative.

$debug = apply_filters( 'prefix_script_debug', SCRIPT_DEBUG );

This will get it out of params but is still testable with WP_Mock easily and controllable in the plugin itself.

Thoughts?

On Friday, December 11, 2015, Brian Watson notifications@github.com wrote:

@lkwdwrd https://github.com/lkwdwrd @sudar https://github.com/sudar

We could preempt the incorrect usage by including a filter and a function that has the correct usage applied.

function admin_scripts( $hook_suffix = '', $debug = false ) {}

This would eliminate the ambiguity and hopefully keep others from applying the wp_enqueue_scripts usage to the admin_enqueue_scripts hook.

— Reply to this email directly or view it on GitHub https://github.com/10up/generator-wp-make/issues/54#issuecomment-164015046 .

Luke Woodward – Engineering Manager 10up Inc. http://10up.com | luke.woodward@10up.com | @lkwdwrd

bswatson commented 8 years ago

If we do that in both admin_scripts and scripts, I'm all for it. It brings the implementation back in line with the expected core usage, but can also be tested as needed.

sudar commented 8 years ago

+1 for filter.

I will get a PR for it sometime this weekend.