I noticed this when running phpcbf on features module.

The module contains a few places with commented-out code. It was using // comments for this, but with weird indentation, I suppose to distinguish them from the rest.
Like so:

function foo() {
  // Inline comment.
//   bar();
}

Obviously, phpcbf tried to "fix" this. But this made the lines blur in with the other inline comments.

I had the idea to replace '//' with '#' for the commented-out line.

function foo() {
  // Inline comment.
  # bar();
}

But then phpcbf cleaned this up again. I suppose based on the comment standards that say

Use of Perl/shell style comments (#) is discouraged.

I think we should change this to:

Use of Perl/shell style comments (#) should only be used for commented-out code - see below.

and then

Commented-out code should be avoided. In most cases it is better to remove those lines of code.
If absolutely necessary, such code should be commented out with perl-style comments (#).

My use case was that the commented-out code was already there, and I only want to remove it once I understand what is going on. Until then, I want to use the perl-style comment to distinguish it from other comments.

Note that we already have / had commented-out code with perl-style syntax in Drupal 7 settings.php.

Comments

donquixote created an issue. See original summary.

drunken monkey’s picture

drunken monkey
he/him
Primary language German
Location Vienna, Austria
commented

Note that we already have / had commented-out code with perl-style syntax in Drupal 7 settings.php.

We also have it there in Drupal 8. That’s why there is a // @codingStandardsIgnoreFile tag at the top of the file.

I suggest you also just use tags to let phpcbf ignore the commented-out code. Adding a new exception for this to our coding standards doesn’t seem worth it, especially if we already discourage commented-out code.
Why not just delete it and, if later necessary, bring it back via Git? That’s what version control is for.

Finally, if we do end up defining an exception for this, I’d strongly prefer // at the start of the line.

But -1 from me.

donquixote’s picture

donquixote commented

Interesting, they now recommend another syntax:
https://github.com/squizlabs/PHP_CodeSniffer/wiki/Advanced-Usage#ignorin...

// phpcs:disable
[..]
// phpcs:enable
donquixote’s picture

donquixote commented

That’s why there is a // @codingStandardsIgnoreFile tag at the top of the file.

This means the entire file is not checked, which in most cases is not desirable.
Luckily there are options to suppress inspection locally, see also my previous comment.
On the other hand, such suppression comments always add noise to a file.

Why not just delete it and, if later necessary, bring it back via Git? That’s what version control is for.

At this point it seemed out of scope to fully remove this comment, because I was not sure what it initially did.
I agree in general it is better to remove code we don't need anymore.

There is one problem with git though: It it is not so easy to spot and blame deleted lines.
There are some tricks, https://stackoverflow.com/questions/4404444/how-do-i-git-blame-a-deleted..., but it is not as straightforward.

Legitimate use cases for commented-out code, even though I might not do it myself:
- Code copied from an "authoritative example", then commenting out lines we modified or removed.
- Code that is meant to be copied and modified, like our default.settings.php.
- Code that is likely to be modified in the future, where options are toggled on and off.
- Legacy code that we want to clean up, without removing commented-out code snippets we don't understand yet.

Finally, if we do end up defining an exception for this, I’d strongly prefer // at the start of the line.

This is in fact what I found in features :)

I don't like it very much, because:
- It looks like a regular inline comment where someone messed up the indentation. So there is a strong urge to "fix" it. And an inspection tool would have the same problem.
- It does not work on file-level code (e.g. templates or settings.php), where regular inline comments also begin at the beginning of the line.

quietone’s picture

quietone commented

So far there is no support for this change. Also, there is an existing issue for Drupal core, to resolve all commented out code that is not part of an @todo. #2909362: [meta] Commented-out code in Drupal. I think those are valid reasons for closing this as won't fix.

I'll ask in @coding_standards.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented

I agree with both @quietone and @drunken monkey, commented out code should not keep living in the source, we should close this issue as won't fix.

quietone’s picture

quietone commented

OK, then closing this as won't fix.

We are currently a small group working on getting Drupal's coding standards moving, we are no official maintainers. Although that is being pursued, #3252921: Add members to and remove members from the Technical Working Group. Join us in #coding-standards in slack if you want to help.

quietone’s picture

quietone commented
Status: Active » Closed (won't fix)