Fix 'Drupal.Classes.PropertyDeclaration' coding standard

Although the above TestBot errors are easily fixed, I think we should not turn this rule on.

Most important reason:

Both \Drupal\views\ResultRow->_entity and \Drupal\views\ResultRow->_relationship_entities are widely used in Contrib (See here and here).
Changing the name of them would lead to massive breakage.

Other reasons:

This sniff is also covered by sniff Drupal.NamingConventions.ValidVariableName.LowerCamelName.

There are two sniff that will do the same task? I am new to this but that seems odd.

AFAIK:

Drupal.Classes.PropertyDeclaration only checks properties and Drupal.NamingConventions.ValidVariableName.LowerCamelName checks all variables for lower CamelCasing.

In the case above both properties start with an underscore, which makes them fail both sniffs.

Would it be possible to enable this sniff but skip it for the files we know we can't change for BC reasons (assuming it only triggers on the declaration, and not each time the property is used)? That way we at least can be sure we won't be introducing any new ones to core, even if we can't fix the existing ones right now.

@longwave in #15

Currently the sniff comes back with this:

FILE: D:\htdocs\drupal\core\lib\Drupal\Core\Config\Entity\ConfigEntityBase.php
--------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
--------------------------------------------------------------------------------------------------------------------------------------------------------
 94 | WARNING | Property name "$_core" should not be prefixed with an underscore to indicate visibility
    |         | (Drupal.Classes.PropertyDeclaration.Underscore)
--------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: D:\htdocs\drupal\core\lib\Drupal\Core\DependencyInjection\DependencySerializationTrait.php
------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
------------------------------------------------------------------------------------------------------------------------------------------------------------------
 18 | WARNING | Property name "$_serviceIds" should not be prefixed with an underscore to indicate visibility (Drupal.Classes.PropertyDeclaration.Underscore)
 25 | WARNING | Property name "$_entityStorages" should not be prefixed with an underscore to indicate visibility
    |         | (Drupal.Classes.PropertyDeclaration.Underscore)
------------------------------------------------------------------------------------------------------------------------------------------------------------------


FILE: D:\htdocs\drupal\core\modules\views\src\ResultRow.php
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------
 15 | WARNING | Property name "$_entity" should not be prefixed with an underscore to indicate visibility (Drupal.Classes.PropertyDeclaration.Underscore)
 22 | WARNING | Property name "$_relationship_entities" should not be prefixed with an underscore to indicate visibility
    |         | (Drupal.Classes.PropertyDeclaration.Underscore)
-------------------------------------------------------------------------------------------------------------------------------------------------------------------------

Time: 4 mins, 2.78 secs; Memory: 44MB

So, as far as I can see, it only triggers on the declaration.

Gonna run with the suggestion from @longwave:

Would it be possible to enable this sniff but skip it for the files we know we can't change for BC reasons [snip]? That way we at least can be sure we won't be introducing any new ones to core, even if we can't fix the existing ones right now.

If we do go down this route, unsure whether we should skip with the config file or start using phpcs:disable comments in the relevant files. We don't use the comment method for PHPCS yet, but we do have some precedent in that we control cspell with code comments in a few places.

#@longwave:

If we do go down this route, unsure whether we should skip with the config file or start using phpcs:disable comments in the relevant files. We don't use the comment method for PHPCS yet, but we do have some precedent in that we control cspell with code comments in a few places.

Pro phpcs:disable:

- It's clear in the code that it won't be checked.

Con phpcs:disable:
- No precedence, but as @longwave said: It has already be done by cspell (which may or may not have a config file in which to exclude files)
- This would mean another change in sniff Drupal.Commenting.InlineComment.InvalidEndChar to ignore //phpcs:disable. (See similiar issue for cspell: #3207576: Problem with Drupal.Commenting.InlineComment.InvalidEndChar: //cspel)

In #3105950: Fix 'Drupal.Commenting.ClassComment.Missing' coding standard I'm about to introduce excluding files using core/phpcs.xml.dist. (Like this)
Since we already have used core/phpcs.xml.dist to exclude files.

I think this is the time to make a decision which way yo go.

Personally: I'm a fan of the config-file excluding style.

I forgot that we do use @codingStandardsIgnoreFile already in a number of places so we do have precedence (and opened #3207968: Replace @codingStandards comments with phpcs: comments to modernise that)

@longwave

Excellent, also we don't have to exclude the full file but some specific lines (https://stackoverflow.com/a/59073074). Let's run with that.

I am not sure we can safely change any of these property names here.

ConfigEntityBase $_core is used for the _core property in exported config entities so I don't think we can easily change that.

$_serviceIds and $_entityStorages are special properties relating to serialization and would need a BC layer for existing serialized objects, we also need to ensure they don't accidentally clash with any existing property name so the underscore helps prevent that.

Looking at @longwave's comment in #25 I think it's better to ignore all three of them.

That way this sniff can go in, and no more of these underscored properties go in.

We could open a follow-up for a BC layer for existing serialized objects for $_serviceIds and $_entityStorages, but that might be too much effort for little result?

Personal opinion is it's not worth changing the serialization properties, they are a really special case and so I think they are OK to stay with their rule-breaking names.

I'm with #TeamLongwave on this one.

I think this is the best we can do here, and it will prevent us adding similar properties in the future without making a good case for doing so first.

This looks good. My only concern is how to prevent someone in the future wanting to remove the ignore lines. Kind of feels like some of the ignores need to have an @see to a doc page with some explanation and history. Not sure though. Is it worth adding an 'exceptions page' to Coding standards?

I like the idea of an explanation/@see for these cases. Having them right beside the ignore bit makes sure people actually see them.

However (untested at the moment) I fear that adding such a line will trigger some of the other sniffs and a Coder/sniff change is needed.

If I have time later today I will give that a test-test-run (See what I did there...), if anybody beats me to it: Fair play :)

Added explanatory comment to each phpcs:ignore

Let's see what TestBot thinks.

If it's green: Let's see what you think => Back to NR.

Also something to consider:

If we like these comments about _why_ phpcs:ignore was needed in the first place, should we create a follow-up to make these comments on the phpcs:ignore that are already in the code.

Also, if we decide to do so, we need to make sure all the sniffs currently in backlog get them.

I applied the MR and looked the changes in PhpSTorm for ConfigEntityBase and ResultRow and found the extra comments informative but it made the code ugly to read. I think we go back to the version that was RTBC in #32. Thanks for making the changes Spokje, I really needed to see it. So, shall we make a single issue follow up for deprecating these class properties. That leaves the code easier on the eye and the details of the properties are not lost.

Thanks @quietone.

I would like more opinions about the comments on phpcs:ignore, I think they're useful and they don't strike me as ugly, but that's in the eye of the beholder I guess.

So, shall we make a single issue follow up for deprecating these class properties. That leaves the code easier on the eye and the details of the properties are not lost.

That's a lot of work to get a BC layer for those in place (See @longwave in #25).

(Again) personally: I don't see that follow-up being worked on in the foreseeable future, since I don't think it will get a lot of priority.

Completely agree with getting more eyes on this!

Regarding the followup, I have voiced my opinion, more than once, and accept the wisdom of the group. No problem there from me. Thanks for listening.

@quietone: More than happy to listen to you, most of the time it makes sense :)

Since my wisdom has long left, I'm also more than happy to listen to The Wiser Ones on this one.

I prefer the version in #32 as well.

I don't think we explicitly need to say why we are ignoring coding standards in the code here. If someone wants to know the reasoning, they can look at git history for the line in question which will link them back to this issue and they can read the discussion here. I find myself doing this for all sorts of core issues where I want to know why a specific line was added.

We add comments to help other developers; I think coding standards comments are a niche case where most developers won't care why the property is named what it is, they are much more interested in the docblock above which tells them what the property is actually used for.

Also as for followups: there is no harm in opening them but as I said in #29 the first three are super-special cases that I think can continue to break the rules. ResultRow is inherited from Views and the naming isn't for any particular reason so we have more chance of fixing this one, but I'm still not sure it's worth the effort.

Thanks @longwave and @quiteone for the input.

Seeing that I'm outnumbered, I can only do what any self-respecting person would do: Surrender! ;)

New MR should contain only phpcs":ignores and the actual adding of the sniff.

Back to RTBC as per #32, I assume @quietone agrees.

Yes, I agree!

And there was much rejoicing!

Committed/pushed to 9.2.x, thanks!