DisallowDefineSniff should only check for top-level define

[gmazzap]

DisallowDefineSniff.php adds and error (!) everytime a define is encountered and the reason (which I fully agree to) is that const should be preferred.

However, in PHP const is not allowed inside condition blocks, because those are only checked at runtime. For example a snippet like the following is invalid and throws a parse error:

if (!defined('FOO')) {
  const FOO = true;
}

Another limitation of const is that it does not accept expression as constant name. For example following is also invalid and throws a parse error:

foreach(['FOO', 'BAR'] as $name) {
   const $name = true;
}

So there are cases in which the usage of define is really necessary, and pretty much it turns out to be anytime the level of const token is not 1.

If you agree, I could send a PR for this.

[sirbrillig]

🤔This is an interesting idea and worth discussing!

I agree that there are uses of define which are useful, but as with any sniff, exceptions can be made by adding // phpcs:ignore NeutronStandard.Constants.DisallowDefine.Define to lines where they occur.

Of course, if there are more than a few exceptions, I understand that this can become tedious. The projects on which I mostly work have few needs for define. If there were such a case, it would be relatively easy to downgrade the sniff to a warning by adding this to the phpcs.xml file:

         <rule ref="NeutronStandard.Constants.DisallowDefine.Define">
        <type>warning</type>
    </rule>

Or by disabling it entirely:

         <rule ref="NeutronStandard.Constants.DisallowDefine.Define">
        <severity>0</severity>
    </rule>

However, I'd like to make the case for not using define in the first place.

The main reason why I find define so troublesome is exactly the cases you describe where the value of a constant is set to a conditional expression. In my opinion constants should simply be aliases for values that do not change. If a constant can have different values depending on various inputs, it's a variable, not a constant. Furthermore, it is a global variable, and global variables can create risky dependencies. In such cases, I think it's better to use an actual value passed down through the dependency tree or at least use a static class variable if it's necessary to reach outside the current scope.

You could make the argument that a constant with a conditional value is constant after it is defined. And that's true, but the problem with that situation is that typically I've seen code use the constant in other conditions. For example, I consider this a code smell:

if (defined('FOOBAR')) {
  // code path A
} else {
  // code path B
}

I know it's common practice, especially in WordPress, but I think that it's a problem because it means the code in the condition is basically un-testable. It's possible to set the constant to one value and thus test either code path A or B, but you cannot test both since most test frameworks operate in a single PHP session and constants cannot be changed.

Instead, one could do this:

if (Options::$foobar) {
  // code path A
} else {
  // code path B
}

In this case, we're using a static variable which is easily changed by tests at runtime.

A good argument against using a variable is that variables can so easily be mutated that it presents a risk to other code. One accidental assignment and our value changes. What we really want is an immutable variable, not a constant. Fortunately, it's actually possible to do something similar with a static function:

class Options {
  private static $foobar = true;
  public static function getFoobar() {
    return self::$foobar;
  }
  public static function setFoobar($newFoobar) {
    self::$foobar = $newFoobar;
  }
}

if (Options::getFoobar()) {
  // code path A
} else {
  // code path B
}

In this case getFoobar() is like an immutable variable. By having setFoobar() available, we make it possible to change the value, but doing so must be explicit and is less likely to be done by accident. We could even put additional safeguards in the setter function if we wished.

Sorry for the long explanation. Please let me know if this makes sense to you and if you disagree. It's very likely that there are cases which I have not considered.

[gmazzap]

I do development for WordPress where bunch of configuration (for either core or plugins) are expected to be done in constants.

I don't like nor advocate it (in my code I try to do configuration via config objects or environment variables), but this is the way it works.

Considering that users might or might not have defined a constant in their configuration file (wp-config.php) the only way to define a constant in case the user has not, it is a construct like

if (! defined('...')) define(...);

Personally, this is the only case I use define and I would need to ignore the code style via comments everytime I use define, which is very annoying. (If I have to ignore a rule every single time it raises a message, the rule doesn't make sense for me).

This is why I disabled this rule in my code style, just like I have disabled other rules, some of them replaced my own version, but I did not open an issue here for those other rules because if they don't fit my code style does not mean they are wrong or should be changed.

The reason why in this case I opened an issue is that there's literally no way that a const with a level > 1 is a valid syntax.

In short, when level > 1 and the sniff says "you should use const here instead of define" if the user follows the sniff suggestion, they ends up in fatal error. And my feeling is that a code standard rule should not suggest users to introduce fatal errors in their code.

Yes, maybe the the define with level > 1 could be replaced with a const in a different place and better architecture of the code, but I honestly think that suggesting code refactoring should not be a concern of a code standard.

By the way, as I said, I already disabled this rule for my own purposes, if you feel this rule is ok as it is, then feel free to close the issue :)