Problem/Motivation

I was working on MR 10884 and accidentally use

#[Hook('hook_node_view')]

instead of

#[Hook('node_view')]

I think this can be catched with static code analysis.
I am not sure if what is best tool for that? phpcs, or phpstan?

Steps to reproduce

Proposed resolution

PR https://github.com/pfrenssen/coder/pull/260

Remaining tasks

User interface changes

API changes

Data model changes

Comments

nikolay shapovalov created an issue. See original summary.

nikolay shapovalov’s picture

nikolay shapovalov commented
Issue summary: View changes
Status: Active » Needs review

PR created https://github.com/pfrenssen/coder/pull/260

nicxvan’s picture

nicxvan commented

The problem is hooks like hook_hook_info.

Now that one won't ever be oop, but we can't guarantee contrib or custom code hasn't done something similar.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

Thanks, approach makes sense, I think we can do this. Left some comments on the PR.

Please add special handling for hook_hook_info(). There are not other hooks in core that would be falsely flagged, I don't think we need to care about other special cases in contrib or custom code (people can make PHPCS ignore directives).

nicxvan’s picture

nicxvan commented

We don't need to worry about hook_hook_info, it cannot be oop anyways.

My point was we shouldn't make assumptions about contrib and custom code.

If you think the ignore rule is enough then it's ok with me, this covers the vast majority of the cases.

nikolay shapovalov’s picture

nikolay shapovalov commented
Status: Needs work » Needs review

@klausi thanks for your review, I update PR and it's ready for review.

@nicxvan, I add special handling for hook_hook_info(). But still not sure about it.

nicxvan’s picture

nicxvan commented

We don't need to handle hook_hook_info it will never be oop.

nikolay shapovalov’s picture

nikolay shapovalov commented

Thanks @nicxvan, I remove special handling for "hook_hook_info()".

nikolay shapovalov’s picture

nikolay shapovalov commented

Decide to improve Hook attribute tests #3501237: Improve HookCollectorPass test.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

Thanks, I think the sniff is almost ready!

nikolay shapovalov’s picture

nikolay shapovalov commented
Status: Needs work » Needs review

Thanks for review, PR updated and ready for the review.

  • d8c10bc4 committed on 8.3.x 
    feat(ValidHookName): Add sniff for hook name in PHP attributes (#3499291...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Fixed

Merged, thanks!

nicxvan’s picture

nicxvan commented

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.