Allow adding hooks to methods that are named starting with `hooks_`

[AndriiSmith]

Expected Behavior

Sometimes, we want to add hooks to a different method than hooks. I think all methods with the hooks_ prefix would be fine for adding hooks.

Current Behavior

We can only add the hooks to the hooks method.

Proposed solution

Replace the condition in the file WPForms/Sniffs/PHP/HooksMethodSniff.php L85 with the following:

        if (
            $tokens[ $function ]['content'] === self::METHOD_NAME ||
            strpos( $tokens[ $function ]['content'], self::METHOD_NAME . '_' ) === 0
        ) {
            return;
        }

[kkarpieszuk]

what are practical cases when you have to add hooks not in hooks() method?

In my cases it happen when you have for some logic suppress filter with remove_filter and then add_filter again.

Are there any other cases?

[wppunk]

IMHO, it's an extra step that we can avoid. In most cases, we add only one callback in some specific method. In this way, adding the phpcs:ignore line is okay. It's just your sign that you understand what're you doing. Similar to the use of $wpdb.

[AndriiSmith]

Let me explain. Sometimes, it happens that there is some setting (let's say "condition") that has a broad influence on the various aspects of code. For example, we are currently working on modern styles of our forms. This means that potentially, in each field, we could have a certain set of filters that changes specific parameters or attributes of tags specifically for modern styles. Yes, of course, we can write a check in each such class, in each hooks method in each Frontend class tied to each Field class. But in my opinion, it would be much more convenient to do such a check once in the base class:

/**
 * Initialize.
 *
 * @since {VERSION}
 */
public function init() {

    $this->hooks();

    if ( wpforms_setting_render_engine() === 'modern' ) {
        $this->hooks_modern();
    }
}

And then, in the Frontend class of the specific field, just use the hooks_modern() to define needed filters and actions only for the modern mode.

Does it make sense?

[ihorvorotnov]

@AndriiSmith for that specific case you’ve mentioned wouldn’t it be simpler to keep all hooks in thehooks method and just use if/else? I’m not sure this case alone justifies splitting hooks and adding new methods. Besides, when we drop classic styles at some point, we’ll have to do a few deprecation backflips to go back to normal naming.

[AndriiSmith]

Ok, I agree. I'll implement using if () {}. Let's close this issue, then.

[kkarpieszuk]

If I can add my two cents:

I understand Andrii's original motivation. Splitting huge hooks() method into smaller ones is a good reason to allow having add_... calls in other methods too. Using if is some solution but it does not prevent us from having huge hooks() method, but it will do the job.

But on the other hand, I agree with Max: for such cases, we have phpcs:disable|ignore. That very well indicates developer knows what he is doing and does not introduce noise in function names.

I would write something like that:

public function hooks() {
  // hook one 
  // hook two
  // ...

  if ( $this->some_condition() ) {
    $this->hooks_for_condition();
  }
}

private function hooks_for_condition() { // phpcs:ignore ...
 // hook three
 // ...
}

And in the case when there are not many conditionally enabled hooks, we could resign from hooks_for_condition and paste them directly into if () {}.

Anyway, I agree to close.

[ihorvorotnov]

@AndriiSmith just a thought re your parent/child relationship... Did you try utilizing PHP for that? E.g. hooks() in parent class, hooks() in subclass and then calling (or not) parent::hooks() when needed? I haven't seen your use case in detail but this approach may help. Or it may not 🙃

[AndriiSmith]

@ihorvorotnov Thanks for the advice. I'm already using PHP classes and subclasses to build my solutions. The issue is not about it, just wanted to expand a little flexibility to improve code readability. Anyway, there is no urgent need for this.

Thanks, everyone, for your attention. I'm closing this issue.