Fix the issues reported by phpcs

Charchil Khandelwal → made their first commit to this issue’s fork.

Patch #2 working fine in my system.
Create MR for this.
Moving it to RTBC +.

Thank you.

Changing the name of the function xmlrpc() to xmlrpc_call() is an API change, so I strongly recommend not to change this, even if it is flagged as a coding standard issue.

The t() calls in tests could be completely removed, t() calls in tests are usually not needed. They could be replaced with strings. For example:

$this->assertSession()->pageTextContains(t('The XML-RPC server returned this response: @num', ['@num' => 77]));

could be replaced with:

$this->assertSession()->pageTextContains('The XML-RPC server returned this response: 77');

ISTR we can quiet the naming warning by moving the function to a .inc file included from the module, can't we ?

@fgm
Based on the coding standard message that sounds like it would work, but I wonder if including an extra file on every request is worth doing for just one coding standard issue (an issue I would call a false positive)?

I think this is an edge case. The function name is the machine name of the module; there is no risk of name conflict because no other module should have a xmlrpc() function.
Plus, there are surely exceptions to how functions are named, for example a module that implements hooks on behalf of another module.

Moving the function in a different file would not have any effect, since the coding standards are valid also for any .inc file a module could use.

Here's a patch to fix the CS issues. I found out that with phpcs:ignore or phpcs:disable you can ignore one specific type of coding standard violation. phpcs:ignore would need to put directly above the line where the coding standard violation is, but that resulted into the coding violation "Missing function doc comment", so that's why I chose to use phpcs:disable instead.

Firstly, phpcs skipped the .module file locally, so I found out that I had to use this command:

phpcs --standard=Drupal . --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml -s

The -s option is to get the sniff code of the violation.

The last patch seems good to go, to me.

Mostly good with using an ignore. However, I think you can put it at the function phpdoc level rather than at the file level, which makes it more focused.

@fgm

However, I think you can put it at the function phpdoc level rather than at the file level

I tried that first, but I got an other coding standard violation in that case: "Missing function doc comment" (see #10).

Did you try putting it at the end of the function line, like this ?

function xmlrpc() { // phpcs:ignore Drupal.NamingConventions.ValidFunctionName.InvalidPrefix

@fgm
I didn't yet, but when I do that:

function xmlrpc($url, array $args, array $headers = []) { // phpcs:ignore Drupal.NamingConventions.ValidFunctionName.InvalidPrefix
  module_load_include('inc', 'xmlrpc');
  return _xmlrpc($url, $args, $headers);
}

I get on phpcs --standard=Drupal . --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml -s:

---------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------------------------------------------------------------
 59 | ERROR   | [x] There should be no white space after an opening "{"
    |         |     (Drupal.WhiteSpace.OpenBracketSpacing.OpeningWhitespace)
 59 | WARNING | [ ] Line exceeds 80 characters; contains 130 characters (Drupal.Files.LineLength.TooLong)
---------------------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------------------------------------------------------------

phpcs:ignore is applied to the next line, not to the line containing it.

added updated patch and address comment #14

+// phpcs:disable Drupal.NamingConventions.ValidFunctionName.InvalidPrefix
+

+// phpcs:ignore Drupal.NamingConventions.ValidFunctionName.InvalidPrefix

Either phpcs:disable or phpcs:ignore is used. The first disables rules on all the lines following the line containing it until the line preceding phpcs:enable, the second disables rules only for the line following the line containing it. (That means phpcs:ignore has been also misplaced.)

I think we'll need to go for the patch in #10 and call it a day. Because putting // phpcs:ignore Drupal.NamingConventions.ValidFunctionName.InvalidPrefix above the function results into an other coding standard violation.

AIUI it needs to be at the end of the line you want to ignore, not above it.

@fgm
You are correct. I see that if I put the ignore on the line it should ignore that coding standard violation is indeed ignored. The issue is though that it results into two other coding standard violations.
In examples I've seen the ignore line to be placed above the line it should ignore. That works too. But in this case that also results into an other coding standard violation: "Missing function doc comment".

So because of these results I think we should disable the rule near the top of the file. Two lines above the function docblock is also a possibility - in that case the "phpcs:disable" line is between xmlrpc_help() and xmlrpc(). Do you like that better, @fgm?

The examples given in the PHP_CodeSniffer documentation are clear.

// phpcs:ignore
$foo = [1,2,3];
bar($foo, false);

// phpcs:disable Generic.Commenting.Todo.Found
$xmlPackage = new XMLPackage;
$xmlPackage['error_code'] = get_default_error_code_value();
// TODO: Add an error message here.
$xmlPackage->send();
// phpcs:enable

It is not true the comment must be in the same line.

Patch added please review.

The last submitted patch, 23: 3329633-23-phpcs_fixes.patch, failed testing. View results →

@chaitanyadessai
Thanks for your contribution, but using $this->t() in tests is rarely needed. So the changes in the tests were already good in the previous two patches.
The only thing were this issue hangs on is where to place the phpcs comment to ignore/disable a coding standard violation.

Options considered:

1. Near the top of the file, using phpcs::disable Drupal.NamingConventions.ValidFunctionName.InvalidPrefix, but that is less focused.
2. Above the function docblock, using phpcs::disable Drupal.NamingConventions.ValidFunctionName.InvalidPrefix, still less focused.
3. Above the function itself, using phpcs::ignore Drupal.NamingConventions.ValidFunctionName.InvalidPrefix, but that causes another coding standard violation: "Missing function doc comment".
4. On the same line as the function, using phpcs::ignore Drupal.NamingConventions.ValidFunctionName.InvalidPrefix, but that causes two other coding standard violations.

I would choose option 1. Options 3 and 4 are out of the question in my opinion as they create new problems.

Okay, looked at this again and I changed my mind. I think from the options listed at #25, option 2 is better. This is the closest focus with the function we can get. And ending with phpcs:enable, so if in the future code is added to the module it checks for the naming conventions again.

Patch #26 seems to be working as expected and can be moved to RTBC.

@Anmol_Specbee
Thanks for checking, committed #26.

Automatically closed - issue fixed for 2 weeks with no activity.

fgm → closed merge request !2