Fix Drupal.NamingConventions.ValidVariableName.LowerCamelName in tests

Removed un-relevant issues from description.

One of the proposed changes by this sniff turns out to be to rename oth \Drupal\views\ResultRow->_entity and \Drupal\views\ResultRow->_relationship_entities.

IMHO that's a bad idea, see here.

We can still use this Sniff, but exclude files that make such impactful changes from being sniffed, like this:

  <rule ref="Drupal.NamingConventions.ValidVariableName.LowerCamelName">
    <exclude-pattern>core/modules/views/src/ResultRow\.php</exclude-pattern>
  </rule>

Another option would be to file an issue in Coder to alter the sniff.
The latter would however mean that the enabling of this sniff would be delayed, possibly considerably.
Which in turn would mean that new code can be committed which offends this Code Standard.

I'm a bit on the fence with this one, but I think I'm leaning slightly towards the first, 'enable-and-exclude'-option.

I too lean to the first option (exclude those files) so that the other fixes can be made. Then, a followup needs to be made so that a solution can be figured out.

If this needs to be skipped every time a variable is *used* rather than just declared I don't think this is viable, because we might introduce new code that needs to use the existing variables and hence would need new exclusions. But I haven't tested the sniff to see how it works yet.

Thanks @quietone and @longwave for their input.

Had a quick play with this one and it's going to be difficult to get into core. The biggest problems are:

• Our annotated classes specify properties with underscores, these map directly on to class properties and we can't change any of this without breaking BC.
• Views has a lot of historical property usage with snake_case names that we similarly can't change.

We don't yet use it in core, but with a combination of // phpcs:disable Drupal.NamingConventions.ValidVariableName.LowerCamelName in the annotated base classes and Views base classes, and something like this in the config file:

  <rule ref="Drupal.NamingConventions.ValidVariableName"/>
  <rule ref="Drupal.NamingConventions.ValidVariableName.LowerCamelName">
    <!-- Views plugins do not conform to this sniff. -->
    <exclude-pattern>**/src/Plugin/views/*/*.php</exclude-pattern>
  </rule>

we might be able to enable this. But there might still be awkward edge cases and I don't know if we should introduce phpcs:disable if we don't need to use it anywhere else?

If we want to break this down first we could also try to introduce this for tests only to start with, and then deal with the rest of core later.

Here is a patch for just the test files. I don't know how to configure phpcs.xml to ignore the non-test files.

There are 39 non test files that fail this sniff, the majority in core/lib/Drupal/Core (15) followed by core/modules/views/src/Plugin (11).

1. +++ b/core/modules/field_ui/tests/src/FunctionalJavascript/ManageDisplayTest.php
   @@ -36,12 +36,12 @@ class ManageDisplayTest extends WebDriverTestBase {
   +  protected $entitytTypeManager;
   

   entitytTypeManager -> entityTypeManager

2. +++ b/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php
   @@ -17,13 +17,13 @@ class CompiledRouteLegacyTest extends UnitTestCase {
   +  private $compileRoute;
   

   Probably should be $compiledRoute?

Right you are.

1 and 2 fixed.

Silly mistake when editing. Fixed that but this is now failing cspell on the word 'uncheck', which is not introduced here. uncheck was removed in #3212547: cspell Dictionaries changed, checking all files.

I re-installed yarn and commit-code-check is passing locally now.

Add link to coding standards in the IS

Reroll and add changes for \Drupal\Tests\jsonapi\Functional\CommentTest.

Will do a better review later

+++ b/core/phpcs.xml.dist
@@ -115,8 +115,6 @@
   <rule ref="Drupal.NamingConventions.ValidVariableName">
-    <!-- Sniff for: LowerStart -->
-    <exclude name="Drupal.NamingConventions.ValidVariableName.LowerCamelName"/>
   </rule>

I think we can change the 4 lines to: "<rule ref="Drupal.NamingConventions.ValidVariableName"/>"

Here is the updated patch from #25.

The testbot is saying that everything is ok.
Only when I run "phpcs -ps" on my local machine I get a lot of warnings.

+++ b/core/tests/Drupal/Tests/Core/Routing/CompiledRouteLegacyTest.php
@@ -17,13 +17,13 @@ class CompiledRouteLegacyTest extends UnitTestCase {
+  private $compiledRoute;

This was renamed from compileRoute but all the usages below in this file were not changed.

The errors in #27 all relate to my comment in #15, we can't change this without some backward compatibility issues. The changes here only affect tests and so if possible we should configure phpcs so only look at tests for this sniff for now.

Working on this

Opened #3224583: The testbot does not run PHPCS on all files when core/phpcs.xml.dist is changed to fix the problem with the testbot.

Ignore This patch.

#3224583: The testbot does not run PHPCS on all files when core/phpcs.xml.dist is changed has landed. We should see now better what is wrong with the patch/MR.

I updated the patch to 10.1.x and the phpcs.xml should be configured to only be doing tests for this sub sniff.

I enabled the sniff without exceptions and the Annotation directories in core/modules were excluded. However, the Annotation directories in core/lib were not. Searching the output in case there were three files found. So, perhaps coder can be modified to exclude these as well.

$ grep FILE a.out| awk -F: '{print $2}'  | grep Annotation | sort -u | nl 
     1   ...html/core/lib/Drupal/Core/Entity/Annotation/ConfigEntityType.php
     2   ...tml/core/lib/Drupal/Core/Entity/Annotation/ContentEntityType.php
     3   ...var/www/html/core/lib/Drupal/Core/Field/Annotation/FieldType.php

I messed up the parent because the other is also about NamingConventions.

The rule has been enabled in the MR.
The testbot returns green.
All code changes look good to me.
For me it is RTBC.

There is a base class altered here, but only views_date_format_sql in contrib extends it and it does not use the renamed properties. Tests are not API, so it's possible that some contrib extends some of these tests, but in most cases it seems unlikely that these properties will be used. To be safe, I committed this to 10.1.x only, even if the test-only changes are eligible for backport. Also adding to release notes as we are enabling a coding standard.