WordPress / WordPress-Coding-Standards

PHP_CodeSniffer rules (sniffs) to enforce WordPress coding conventions
MIT License
2.56k stars 487 forks source link

Errors regarding usage of deprecated core functions are too strict in a context of a namespace #1244

Open markkap opened 6 years ago

markkap commented 6 years ago

In the following code

namespace dummy;

function get_settings() {
   return 'settings';
}

echo get_settings();

You are going to get a notice about the usage of the deprecated function get_settings() although the one being called in this code is the "local" one.

Suggested solution, when in the context of a namespace look for the existence of a "\" before the deprecated function name.

jrfnl commented 6 years ago

@markkap Thanks for reporting this. A lot of sniffs have been written with PHP 5.2 support in mind and they don't all handle more modern code 100% correctly, so this will be a good example to use to fix this.

Related to #764

jrfnl commented 6 years ago

@markkap I've just had a quick look at this and I can't think of a simple solution.

The problem is that - unless, like in your example, the namespaced function definition is in the same file as the function call -, there is no way to reliably know whether that function is defined within the namespace or not.

If not, PHP will fall back on the global function which would mean, it would use the deprecated function after all.

For functions and constants, PHP will fall back to global functions or constants if a namespaced function or constant does not exist.

Ref: http://php.net/manual/en/language.namespaces.fallback.php

markkap commented 6 years ago

@jrfnl, yes :(

OTOH if it is impossible to know which function is being referred to, it is hard to say that you are calling a deprecated one.

Maybe what can be done is to turn off this check when there is a namespace "use". get_settings is just an obvious generic name that should not generate errors, maybe for other deprecated functions it is less of an issue.

jrfnl commented 6 years ago

@markkap The error will not be turned off in WPCS itself, though I will look into checking use statements at some point.

In the mean time, you can turn it off for your own projects by adding <exclude name="WordPress.WP.DeprecatedFunctions.get_settingsFound"/> to your custom ruleset.

markkap commented 6 years ago

The problem is not so much with me or the other few people that right now use WPCS and actually understand how things work and why, the problem is the move to use it to grade plugin and theme code, and if there are too many false positives it is a big problem, especially when it is more likely to happen in the context of code which is compliant with the newer PHP versions.

On Thu, Mar 1, 2018 at 2:40 PM, Juliette notifications@github.com wrote:

@markkap https://github.com/markkap The error will not be turned off in WPCS itself, though I will look into checking use statements at some point.

In the mean time, you can turn it off for your own projects by adding <exclude name="WordPress.WP.DeprecatedFunctions.get_settingsFound"/> to your custom ruleset.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1244#issuecomment-369579342, or mute the thread https://github.com/notifications/unsubscribe-auth/ABJnmnpBng0uHCN5awNaT6gGrLzhTKyXks5tZ-w4gaJpZM4QswRd .

jrfnl commented 6 years ago

@markkap Methods will not generate false positives, it's just namespaced functions without the namespace prefix, so I wouldn't expect that to have much of an impact.

markkap commented 6 years ago

:( namespaced functions in php do not have to have any prefix. That is the whole point of having the "use" construct. Right now WPCS will fail even if the the RFC which suggests to eliminate the fallback to global scope will be accepted (something that I doubt very much that will happen). It is a false positive, you can call it too minor to matter, but it is what it is ;)

On Thu, Mar 1, 2018 at 6:31 PM, Juliette notifications@github.com wrote:

@markkap https://github.com/markkap Methods will not generate false positives, it's just namespaced functions without the namespace prefix, so I wouldn't expect that to have much of an impact.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/WordPress-Coding-Standards/WordPress-Coding-Standards/issues/1244#issuecomment-369649319, or mute the thread https://github.com/notifications/unsubscribe-auth/ABJnmjZBVDEo4LHzA4lPoas2emNQYMmIks5taCJ1gaJpZM4QswRd .

jrfnl commented 6 years ago

@markkap I already said, I will look into checking use statements at some point.

Other than that, you're welcome to try and find a workable solution yourself. I'd be happy to look a PR over.

jrfnl commented 4 years ago

Tentatively marking this for WPCS 3.0. PHPCSUtils will contain an Abstract sniff for function call detection which is better than the one we are currently using and would, at least partially solve this.