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

Warning for translate triggers class function #719

Closed
thomasplevy opened this issue Dec 2, 2016 · 8 comments · Fixed by #1318
Closed

Warning for translate triggers class function #719

thomasplevy opened this issue Dec 2, 2016 · 8 comments · Fixed by #1318

Comments

@thomasplevy
Copy link

thomasplevy commented Dec 2, 2016

I have a class function in my plugin:

abstract class myclass { public function translate( $key ) {} }

This function doesn't do anything by default, but allows extending classes to pull a database value by $key and translate it. The extending classes ultimately run through some WP core i18n function like __()

During PHPCS runs this function triggers the i18nSniff error: The $text arg should be single a string literal, not "$key".

I see that some sniffs -- the "DontExtract" for example -- seem to allow this pattern, but this translate function doesn't follow the pattern

I tried to figure out how to rewrite the tests to allow for this exception to submit a PR but after some digging and attempts I decided I am not qualified to do so. Sorry...

Thanks,

@westonruter
Copy link
Member

The problem in the i18nSniff is that it is not distinguishing between this example of defining a method and a function call for translate(). So the sniff needs to look at the context to skip if it is not a function call context. Probably the easiest way to check is to see if there is an opening brace following translate(), and if so, skip it.

@GaryJones
Copy link
Member

If the code was formatted as per WP coding standards, instead of all on one line, would the supposed (but incorrect) lack of i18n be flagged still?

@jrfnl
Copy link
Member

jrfnl commented Dec 2, 2016

@westonruter FYI: I'm working on a number of utility functions which I hope to introduce into WPCS soonish, one of these will be to determine if a T_STRING is a function call (rather than a function definition, method call etc). I think using that utility function near the top of the sniff to determine whether the function arguments should be inspected, will solve this issue.

@thomasplevy
Copy link
Author

thomasplevy commented Dec 3, 2016

@GaryJones

The function doesn't actually look like that anywhere in the codebase, just gave a simply one-liner as an example

The actual function in the abstract is:

public function translate( $key ) {
     return '';
}

@GaryJones
Copy link
Member

Right - so does the same error still happen when it's formatted correctly?

I just want to ascertain that the issues is the naming of your method that is causing the issue, rather than the way the code is formatted.

@thomasplevy
Copy link
Author

@GaryJones -- is the above function formatted incorrectly?

@GaryJones
Copy link
Member

public function translate( $key ) {
     return '';
}

is correctly formatted, yes.

@westonruter
Copy link
Member

<?php
class Foo {
	public function translate( $key ) {
		return '';
	}
}

Yields:

--------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------
 3 | WARNING | Use of the "translate()" function is reserved for low-level API usage.
 3 | ERROR   | The $text arg should be single a string literal, not "$key".

This warning and error are bugs.

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

Successfully merging a pull request may close this issue.

4 participants