PHPCSStandards / PHP_CodeSniffer

PHP_CodeSniffer tokenizes PHP files and detects violations of a defined set of coding standards.
BSD 3-Clause "New" or "Revised" License
856 stars 53 forks source link

Generic/CallTimePassByReference: add support for self, parent and static #466

Closed rodrigoprimo closed 3 months ago

rodrigoprimo commented 4 months ago

Description

This PR changes the Generic.Functions.CallTimePassByReference sniff to trigger an error when the parent, self, or static tokens are used to instantiate a class containing a variable passed by reference. In a separate commit, it also sorts alphabetically the list of tokens this sniff listens to.

Suggested changelog entry

Generic.Functions.CallTimePassByReference: flag call-time pass-by-reference when instantiating a class using parent, self, or static.

Related issues/external references

Suggested in https://github.com/PHPCSStandards/PHP_CodeSniffer/pull/394#pullrequestreview-2015022891

Types of changes

PR checklist

rodrigoprimo commented 4 months ago

Thanks for the review, @jrfnl!

As a side-note: I'm not a fan of always sorting the tokens in register() alphabetically. On the one hand it makes history tracing more difficult; on the other hand, the tokens are often sorted by the kind of language structure they represent, so sorting them increases the cognitive load.

Sorting makes sense when all tokens represent the same type of language structure, but that's not the case here.

Noted. Let me know if you prefer that I drop the commit that sorts the tokens in this PR.

jrfnl commented 4 months ago

Let me know if you prefer that I drop the commit that sorts the tokens in this PR.

Well, I've said what I had to say about it. It's now up to you to decide what to do with it.

rodrigoprimo commented 4 months ago

My impression is that it is better to sort the tokens alphabetically for all the sniffs. That being said, I have way less experience than you, so I trust your opinion more than mine on this matter. I went ahead and dropped the commit. I added the tokens to the end of the list grouped together with the T_ANON_CLASS as all those tokens relate to classes and objects. I'm not sure if that is the organization that you had in mind.

jrfnl commented 3 months ago

Rebased without changes. Merging once the build passes.