Use class constants for hooks names

[gmazzap]

As per discussion in #11, many hooks are used in different places, we should use class constant for them

[gmazzap]

Pretty much all hoks names are no in class constants. The "main" hook wonolog.log is in the constant Inpsyde\Wonolog\LOG so it does no belog to any class. In PHP 5.6+ with use constant this hook can be fired like do_action(LOG, ... ) which is quite fine, I think.

Existing hooks won't break because the constants use same string, but is suggested to replace theme.

[dnaber-de]

How does PHPStorm behave on that? Doesn't that break the integration of the WP Hook system in a case that I can't easily «jump» to an origin of a hook form the place it is assigned?

[gmazzap]

At the moment it breaks, yes.

I implemented this because it was discussed in last call, at seemed there was consensus on this.

I can tell that being a constant you can leverage "find usages" which -at least- prevents you to search manually for where the hook is fired.

But I guess we can open an issue on phpStorm to ask them to implement search hook by constant, that should not be hard for them.

[dnaber-de]

I implemented this because it was discussed in last call, at seemed there was consensus on this.

This seems to passed me completely 😕
What was the benefit again?

I can tell that being a constant you can leverage "find usages" which -at least- prevents you to search manually for where the hook is fired.

At least I cannot use the constant directly in an independent plugin, as this would completely negate the flexibility of hooks that are applied/fired if available but nothing happens if for example Wonolog isn't available. Maybe I get something wrong here?

[gmazzap]

I get something wrong here?

No, you don't.

When we talked, @tfrommen pointed out that hooks that are used in many places in the plugin should be wrote in a constant, and I agreed.

Reason why I agreed is that it is hard to remeber hooks (expecially long named hooks) and it's easy to fat-finger an hook name. Moreover, more generally speaking, to have core business rules that rely on a weak type like string is wrong IMO, more so on a package that supports PHP 5 where string can't event be enforced as type, so I always prefer to have a constant or even a value object instead of strings when they affect or dictate business logic.

Is it also true what you say: phpStorm WordPress integration is better (at least, for now) when you use hardcoded strings for hooks.

Now, I like phpStorm and its WP integration, but I value more compliance with better code (or my idea of better code) than a specific IDE helpers. Even because if there's an issue with IDE it should be fixed in the IDE and should not affect the code, IMO.

This is my opinion, if there's a consensus against it I am ok to revert this change.

[dnaber-de]

This is my opinion, if there's a consensus against it I am ok to revert this change.

Please don't. But before we deploy it to other projects, we should talk about it with more people, I think. I'm still confused that I missed that part of the meeting out. :/

When we talked, @tfrommen pointed out that hooks that are used in many places in the plugin should be wrote in a constant, and I agreed.

If you use a hook name more than once it might be a good idea to capsule it in a function or better in an API method. That also solves all the other problems like weak types or fat-finger hook names.

[gmazzap]

If you use a hook name more than once it might be a good idea to capsule it in a function or better in an API method. That also solves all the other problems like weak types or fat-finger hook names.

We get back to initial issue: API forces dependency to Wonolog. Directly triggering the action, doesn't.

Is it true that in other places you can't use the constant, but in other plugins you probably don't get the IDE helper either, so you might need write it manually (or copy and paste from documentantation) either way.

If you have an API you could do

function_exists( 'Inpsyde\wonolog_log' ) and Inpsyde\wonolog_log( ... )

but at this point why not

defined( 'Inspyde\Wonolog\LOG' ) and do_action( Inspyde\Wonolog\LOG ... )

(the latter is also faster)

But being an external plugin, I see no problems in just do do_action( 'wonolog.log' ... ) (this is what would be documented).

Using a constant is something that makes package (Wonolog) code better... how consuming code make uses of the package is not up to the package.

For example, a package could define own function to trigger wonolog log actions, and that that would be totally fine. For example:

namespace My\Package;

function log( $log_data ) {

  static $wonolog_hook;
  if ( ! isset($wonolog_hook) ) {
    $wonolog_hook = defined('Inspyde\Wonolog\LOG') ? Inspyde\Wonolog\LOG : '';
  } 

  if ($wonolog_hook) {
    do_action( $wonolog_hook, $log_data );
    return;
  }

  error_log( json_encode( $log_data ) );
}

But this is something that is up to consumers, and not to wonolog, IMO.

[tfrommen]

Basically, everything is said already. :)

That some string is available through a class constant that you can reference, does not mean at all that you only can reference the value via the constant. You can (and are good to) still use the string directly.

If you want to reference the class constant (and you didn't check for Wonolog being active before), then you have to make sure you can do this. For example, just like @gmazzap illustrated, by using defined() and do_action().

The benefit is that you have the value in a single place. If it ever were to change, you only have to update the constant - and all code that already references the constant will work just fine.
I like to do this thing on interfaces. They provide action and/or filter hook names that implementors can (and should) use. So, it's not always that you have multiple parties use this in parallel - but maybe one out of several possible versions. Having some wrapper function in this case would just be an unnecessary overhead (although negligible, of course).
You also get auto-complete in your IDE.
And lastly, your test can directly use the class constants - and doesn't have to be adapted anytime.

But: yes, it would also be cool to teach PhpStorm constant action/filter look-up. I will have a look at how best to do this - maybe only open a youTrack issue.