Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sniff idea: Detect when constant is being used in constant() #717

Open
rebeccahum opened this issue Jul 13, 2022 · 3 comments
Open

Sniff idea: Detect when constant is being used in constant() #717

rebeccahum opened this issue Jul 13, 2022 · 3 comments

Comments

@rebeccahum
Copy link
Contributor

Describe the solution you'd like

Not sure if VIPCS is the best way forward, but I think this would be a worthy sniff.

When using constant(), we should throw a warning if a string is not being inputted. Of course it is valid PHP to do something like:

constant( FOO_BAR );

But I think most likely, what the user wants to do is:

constant( 'FOO_BAR' );

What code should be reported as a violation?

constant( FOO_BAR );

What code should not be reported as a violation?

constant( 'FOO_BAR' );
constant( "FOO_BAR" );
constant( $foo_bar );
@jrfnl
Copy link
Collaborator

jrfnl commented Jul 14, 2022

Hmm... while I understand where you are coming from, I don't think this is a good idea.
The constant() function should only be used when the name of the constant isn't known or is variable, so if it is known - aka is a text string -, devs shouldn't be using the constant() function, but should be using the constant directly.

When I - as a dev - look at the above code samples, I would say that:

  • constant( FOO_BAR ); and constant( $foo_bar ); should NOT be a violation (though the first should be very rare anyway and mostly in the form of constant( static::FOO_BAR ); for a (child) class to be allowed to overrule the constant name being used.
  • while constant( 'FOO_BAR' ); and constant( "FOO_BAR" ); should be a violation as the dev should just use the constant directly.

Ref: https://www.php.net/manual/en/function.constant.php

@rebeccahum
Copy link
Contributor Author

while constant( 'FOO_BAR' ); and constant( "FOO_BAR" ); should be a violation as the dev should just use the constant directly.

Agreed. But in instances like our vip-go-mu-plugins, sometimes we have to call constant( 'FOO_BAR' ) in our code to make our unit tests happy: https://github.com/Automattic/vip-go-mu-plugins/blob/fdd99cf535e55eeda46d7a11a4cbb1603d8bc666/tests/mock-constants.php. So, doing that can result in us having to go against best practices in how constant() is used :/.

@jrfnl
Copy link
Collaborator

jrfnl commented Jul 18, 2022

But in instances like our vip-go-mu-plugins, sometimes we have to call constant( 'FOO_BAR' ) in our code to make our unit tests happy: https://github.com/Automattic/vip-go-mu-plugins/blob/fdd99cf535e55eeda46d7a11a4cbb1603d8bc666/tests/mock-constants.php. So, doing that can result in us having to go against best practices in how constant() is used :/.

@rebeccahum Just had a look, but in those cases, you are not calling the PHP native global constant() function, but a namespaced function called constant().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants