Ignore Diff component files in phpcs coding standards

After looking this over, I think we should potentially add this to each file in the diff component, so expanding the scope slightly.

So yah, the backstory on the diff component is that it was copied into core from the diff contributed module in #120955: Integrate Diff into Core, which in turn used the diff library from PhpWiki, which in turn is also used and maintained by MediaWiki (a.ka. Wikipedia) now (or at least as of 2012). The intent at the time was to make only minimal changes to keep the code as close as possible to the previous library, and do #1848264: Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php as a followup with the hope being that we wouldn't have to maintain it in the future. However, that followup issue hasn't been finished in the four-and-some years since. :)

Since we refactored the diff component for PSR-0, the vast majority of changes made (aside from removing SafeMarkup) are just coding standards cleanups from patches like #2842949: Fix Drupal.Commenting.DocComment.SpacingBeforeTags coding standard. Each time we fix the coding standards to conform to our standards, we take the library further away from the library we want to merge it with.

So, I think it's better to skip checking the files in the component until #1848264: Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php is addressed one way or another.

Thanks @sugaroverflow! I confirmed that the patch adds the ignore comment to all the classes in the Diff component namespace. To test that it was working, I changed one of the classes to use the old array() syntax and confirmed that coding standards checks failed in HEAD with that change, but passed with the patch applied.

So I think this patch is RTBC.

Adding the issue tag we belatedly agreed on. :)

Instead of adding it to the files why not exclude them in core/phpcs.xml.dist?

It's not a pattern of file that needs to be excluded; it's specific files. core/phpcs.xml.dist has only these two exclusions:

  <!--Exclude third party code.-->
  <exclude-pattern>./assets/vendor/*</exclude-pattern>

  <!--Exclude test files that are intentionally empty, or intentionally violate coding standards.-->
  <exclude-pattern>./modules/system/tests/fixtures/HtaccessTest</exclude-pattern>

Whereas we have this many files with a @codingStandardsIgnoreFile comment already in HEAD:

core/lib/Drupal/Core/Archiver/ArchiveTar.php
core/lib/Drupal/Core/Command/DbDumpCommand.php
core/lib/Drupal/Core/Command/GenerateProxyClassCommand.php
core/lib/Drupal/Core/DependencyInjection/ContainerBuilder.php
core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
core/lib/Drupal/Core/ProxyClass/Batch/BatchStorage.php
core/lib/Drupal/Core/ProxyClass/Config/ConfigInstaller.php
core/lib/Drupal/Core/ProxyClass/Cron.php
core/lib/Drupal/Core/ProxyClass/Entity/ContentUninstallValidator.php
core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php
core/lib/Drupal/Core/ProxyClass/Extension/RequiredModuleUninstallValidator.php
core/lib/Drupal/Core/ProxyClass/Field/FieldModuleUninstallValidator.php
core/lib/Drupal/Core/ProxyClass/File/MimeType/ExtensionMimeTypeGuesser.php
core/lib/Drupal/Core/ProxyClass/File/MimeType/MimeTypeGuesser.php
core/lib/Drupal/Core/ProxyClass/Lock/DatabaseLockBackend.php
core/lib/Drupal/Core/ProxyClass/Lock/PersistentDatabaseLockBackend.php
core/lib/Drupal/Core/ProxyClass/PageCache/ChainResponsePolicy.php
core/lib/Drupal/Core/ProxyClass/ParamConverter/AdminPathConfigEntityConverter.php
core/lib/Drupal/Core/ProxyClass/ParamConverter/MenuLinkPluginConverter.php
core/lib/Drupal/Core/ProxyClass/Plugin/CachedDiscoveryClearer.php
core/lib/Drupal/Core/ProxyClass/Render/BareHtmlPageRenderer.php
core/lib/Drupal/Core/ProxyClass/Routing/MatcherDumper.php
core/lib/Drupal/Core/ProxyClass/Routing/RouteBuilder.php
core/modules/book/src/ProxyClass/BookUninstallValidator.php
core/modules/field/src/ProxyClass/FieldUninstallValidator.php
core/modules/filter/src/ProxyClass/FilterUninstallValidator.php
core/modules/forum/src/ProxyClass/ForumUninstallValidator.php
core/modules/language/src/ProxyClass/LanguageConverter.php
core/modules/migrate_drupal/tests/fixtures/drupal6.php
core/modules/migrate_drupal/tests/fixtures/drupal7.php
core/modules/node/src/ProxyClass/ParamConverter/NodePreviewConverter.php
core/modules/simpletest/files/php-1.txt
core/modules/simpletest/files/php-2.php
core/modules/system/tests/fixtures/update/drupal-8.language-enabled.php
core/modules/system/tests/src/Kernel/Scripts/DbDumpCommandTest.php
core/modules/user/src/MigratePassword.php
core/modules/user/src/UserServiceProvider.php
core/modules/views_ui/src/ProxyClass/ParamConverter/ViewUIConverter.php
core/tests/Drupal/Tests/Component/EventDispatcher/ContainerAwareEventDispatcherTest.php

So, if we want to discuss changing where we mark the files as ignored, let's do that in a separate policy issue, but for now, let's follow the 95% pattern.

Thanks @jibran!

I guess excluding an entire namespace would be simpler using the rules instead of a per-file comment, and also allows adding comments explaining why at a high level. We can see a similar thing with the ProxyClass exclusions above. @jibran, want to file a separate issue to convert to that pattern throughout core? Then we can discuss whether to do that. For this though we can go ahead with what we currently use I think.

Okay I thought about it more and this patch would add 25% more ignored files to core, so maybe it's a good idea to agree to do that first rather than expanding the scope of what we might change later, if we agree tracking them in the rule file is preferable. So based on @sugaroverflow's patch, the rule we add would be something like this? Edit: Note that I did not actually review/test this.

We could then (in a followup issue, not this one) add (e.g.) ArchiverTar to that same grouping. The generated proxy classes need to retain the per-file comment, since they are generated and can exist anywhere. Others might need to be investigated on a case-by-case basis though, which is why I was originally reluctant to change the scope here and why it should have a separate issue.

Alright, I did confirm that #11 and #4 work equivalently.

We could then (in a followup issue, not this one) add (e.g.) ArchiverTar to that same grouping. The generated proxy classes need to retain the per-file comment, since they are generated and can exist anywhere. Others might need to be investigated on a case-by-case basis though, which is why I was originally reluctant to change the scope here and why it should have a separate issue.

I created #2891586: Remove @codingStandardsIgnoreFile from file and ignore those in phpcs.xml.dist. Let's bikeshed there. I have an idea for proxy classes.

@xjm this is some amazing work. Thanks, for considering #8 and coming up with the nice one line fix. :)

Thanks @jibran!

I noticed when reviewing #2891586: Remove @codingStandardsIgnoreFile from file and ignore those in phpcs.xml.dist that I missed a space between the <!-- and the text of the comment; fixed in attached. I left it RTBC and didn't add an interdiff because it's not that inter-different when it's a two-line patch. :P

I think this also should be backported so that we don't diverge these diff files between branches, so setting the branch accordingly. Also saving isse credit for @jibran.

Still looks good to me. RTBC +1.

Ahem yep, actually fixing the space in the comment. Good catch @vaplas. I guess phpcs just ignores a malformatted tag.

I wondered the same thing about the path format in #2891586: Remove @codingStandardsIgnoreFile from file and ignore those in phpcs.xml.dist. I included the trailing slash on purpose, so that we don't accidentally skip (e.g.) a Drupal\Component\DiffFoo namespace that might be added in the future. I left the asterisk off because it seemed to be irrelevant. The case I could see for including it might be that it's more readable/semantic, but it does not seem to affect the functionality of the rule.

@vaplas, You caught an error in the patch (even if it was just one transposed character) and asked a helpful clarifying question that might aid a committer's review, so you will probably receive credit unless you really adamantly ask us not to. :)

If we may remove the exception for this namespace based on the results of #1848264: Compare and merge PhpWiki diff*.php with MediaWiki's DairikiDiff.php and DiffEngine.php, shouldn't we have a todo referencing it somewhere?

@cilefen, sure, thanks. Attached adds the @todo above the rule.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!