Permalink
Browse files

add filter to get hash of full CSS (different one per media-type though)

  • Loading branch information...
futtta committed Nov 27, 2017
1 parent 1e58f67 commit fae43315d5b21387082962a9c8f56acccc88e4f2
Showing with 1 addition and 0 deletions.
  1. +1 −0 classes/autoptimizeStyles.php
@@ -327,6 +327,7 @@ public function minify() {
foreach($this->csscode as &$code) {
// Check for already-minified code
$hash = md5($code);
apply_filters('autoptimize_filter_css_hash', $hash);
$ccheck = new autoptimizeCache($hash,'css');
if($ccheck->check()) {
$code = $ccheck->retrieve();

6 comments on commit fae4331

@zytzagoo

This comment has been minimized.

Show comment
Hide comment
@zytzagoo

zytzagoo Nov 27, 2017

Collaborator

This calls apply_filters() but doesn't store the result, so the line below does not see the new (potentially filtered) value of $hash.

Which makes no sense if the reason for this addition is to allow $ccheck = new autoptimizeCache($hash,'css'); to use the filtered value.

If, however, the intention is to "only" provide a hook for other code to just do something based on the value of $hash, perhaps a do_action() call is more appropriate? (albeit with a different action name of course)

Or am I missing something?

Collaborator

zytzagoo replied Nov 27, 2017

This calls apply_filters() but doesn't store the result, so the line below does not see the new (potentially filtered) value of $hash.

Which makes no sense if the reason for this addition is to allow $ccheck = new autoptimizeCache($hash,'css'); to use the filtered value.

If, however, the intention is to "only" provide a hook for other code to just do something based on the value of $hash, perhaps a do_action() call is more appropriate? (albeit with a different action name of course)

Or am I missing something?

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Nov 27, 2017

Owner

the goal is to make the different hashes available read-only, hence it is not assigned as new value of $hash. does add_action/ do_action have tangible advantages here (except for the fact it's weird to see a filtered value go to waste like that)? :-)

Owner

futtta replied Nov 27, 2017

the goal is to make the different hashes available read-only, hence it is not assigned as new value of $hash. does add_action/ do_action have tangible advantages here (except for the fact it's weird to see a filtered value go to waste like that)? :-)

@zytzagoo

This comment has been minimized.

Show comment
Hide comment
@zytzagoo

zytzagoo Nov 27, 2017

Collaborator

I think do_action() would reduce confusion for future source readers.

Filters must return things, and are generally used when you can modify the way something works/functions.
Since you cannot change the way AO works here, and you are only getting "notified" about what's going to happen, the intention would be more clearly communicated using do_action IMO.

https://htaccess.wordpress.com/2007/12/13/add_filter-and-add_action/

Collaborator

zytzagoo replied Nov 27, 2017

I think do_action() would reduce confusion for future source readers.

Filters must return things, and are generally used when you can modify the way something works/functions.
Since you cannot change the way AO works here, and you are only getting "notified" about what's going to happen, the intention would be more clearly communicated using do_action IMO.

https://htaccess.wordpress.com/2007/12/13/add_filter-and-add_action/

@chesio

This comment has been minimized.

Show comment
Hide comment
@chesio

chesio Nov 29, 2017

I would only add that filters should (ideally) have no side-effects, so given the goal, I also believe do_action() should be used here.

chesio replied Nov 29, 2017

I would only add that filters should (ideally) have no side-effects, so given the goal, I also believe do_action() should be used here.

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Nov 29, 2017

Owner

has indeed been changed to do_action @chesio :-)

Owner

futtta replied Nov 29, 2017

has indeed been changed to do_action @chesio :-)

@chesio

This comment has been minimized.

Show comment
Hide comment
@chesio

chesio Nov 29, 2017

My bad, I should have checked before chiming in... Will do the next time! :-)

chesio replied Nov 29, 2017

My bad, I should have checked before chiming in... Will do the next time! :-)

Please sign in to comment.