Fix coding standards

How about a patch to fix just the array syntax in the TESTS? As far as I can see, this won't disrupt any pending patches, and there's no chance of messing up the operation of the module since these are just tests. This would fix more than 600 of the current > 1600 (!) coding standards errors, and help make it easier to move the Simpletests to PHPUnit.

As far as I can see, this won't disrupt any pending patches

I'm fairly sure there are issues in needs work or needs review state that alter tests.

My statement was made after I looked at the more than half of the NR and NW issues in the queue and I didn't find any that modified or added tests. If they don't modify or add tests, then it would be impossible for syntax changes to the test cases to affect those patches. While it's possible there are some issues this will disrupt, I'm confident that MOST will not be affected at all. And if there are some pending patches that need to be re-rolled after coding standards fixes, then I'm perfectly willing to do that myself - it's a pretty trivial task. I don't see how putting this off until later will make it any easier.

How about a patch to fix just the array syntax in the TESTS?

I'm not attracted to that as it seems like it will make two separate occasions when we cause disruption.

I don't follow your logic. Addressing just the test cases won't be disruptive - that's the whole point of doing just the tests - but it will make it easier to finish the job in a non-disruptive manner. For example, non-test code could be addressed on a class-by-class basis, and we could then fix only the classes that DON'T have pending patches. This allows us to tackle these issues bit-by-bit without any disruption at all.

help make it easier to move the Simpletests to PHPUnit

Please can you explain how it would help with that or any other reason why there is any urgency to fix this?

First, note that this is a "Task" of "Normal" priority. There is no claim that this is a bug or that there is any urgency to it.

Second, there are various unofficial automated tools (used by Drupal core) which can mostly transform Simpletests to PHPUnit tests. But these tools are fairly fragile - they make certain assumptions such as the code conforms to Drupal coding standards. Moving tests to PHPUnit has to be done before D9, and Simpletests have already been deprecated for several years. I don't see how putting things off makes the project easier to manage - more likely if all this maintenance is deferred then there will just be too much work to do all at one time and this module will never see a D9 port.

Third, I'm not going to get into a debate about coding standards - the Drupal community has adopted them, and like it or not they are part of the job of maintaining a Drupal module. It's not a lot of work to do this, especially if there are community members willing to do the work. But I can tell you that I find adhering to coding standards helps a lot by making the automated tests more useful - when you have 1600 coding standards errors (most of which are easy to fix or false positives), it makes it hard to pick out the one or two errors which may actually indicate real problems. I also find that cleaning up the code makes development easier, because then searches can work the same across the entire code base and because automated tools and IDEs are more powerful when the infinite variations that occur in valid code are restricted to a known subset.

I understand you not wanting to do this work yourself, since it doesn't necessary make any functional difference in the module's operation, but I don't get why you would want to put this off when others are offering to do the work for you.

So if you're open to a patch that fixes these issues in tests only, then let me know - I have a patch waiting and ready to commit.

There are way too many errors for this to be easy to review.

I propose a first patch that is completely automated, followed by a second (or more) patch that requires human intervention.

This is the first fully automated patch, resulting from running phpcbf --standard=Drupal --extensions='php,module,inc,install,test,profile,theme,js,css,info,txt'.

Once this patch is committed, the followup patch is way easier to review, as it will not contain the ~9000 lines of code in this patch.

https://www.drupal.org/node/1587138#automatically-fix-coding-standards

I'm including js, but no js file was changed, and I didn't include md files.. But there are no md files in this project anyway. I've updated my alias, but the output remains exactly the same as in #11.

No worries, this was just a simple first step. I can recreaete the above patch in 5 minutes.

Any clues why the tests fail, though? I haven't been able to look at the 9000 lines, but this shouldn't break the tests.

Let's try a simpler one.. This is just running the sniff for the short array syntax. The patch is just over 7K lines long.

No interdiff, because there's no point. Except to say that this was generated using:

drupalcbf --sniffs=Drupal.Arrays.DisallowLongArraySyntax .

Setting to RTBC, because the tests passed and these are 7000 lines of replacing array() with [].

@AdamPS Please commit this one whenever you can, so that we can move on the the other rules.

Totally understood.

If you want, you can also run it yourself, there's a very easy guide to setup the code sniffer yourself here:
https://www.drupal.org/docs/8/modules/code-review-module/installing-code...

In the end, it describes how to use it: https://www.drupal.org/node/1587138

As I said in #18, to generate the above patch, I ran drupalcbf --sniffs=Drupal.Arrays.DisallowLongArraySyntax . in the module directory after a checkout.

I'll try to add a few other sniffs that should also be completely harmless to see if they don't break the tests. If they break, at least I'll know what is breaking the tests.

OK. This one is ~8.5K long and is the result of running drupalcbf --sniffs=Drupal.Arrays.DisallowLongArraySyntax,Drupal.Scope.MethodScope,Drupal.WhiteSpace.ScopeIndent,Drupal.Classes.ClassDeclaration,Squiz.WhiteSpace.FunctionSpacing,Drupal.Commenting.InlineComment,Drupal.Arrays.Array,Drupal.Commenting.FunctionComment .

Note that the test failure in #24 is an intermittent failure - re-running the test once or twice will probably result in a success. Fixing that intermittent failure should be a separate issue, and that failure should not be blamed on this patch. (You can verify this by looking at the test results for a much different patch - https://www.drupal.org/pift-ci-job/1399305 - where you will see that the test was run twice, failing the first time with the exact same error as #24 then passing the second time without changing the patch at all.)

Looking at the patch in #24, the very first change is:

-
-/*
- * Implements hook_install().
+/**
+ *
+ *Implements hook_install().
  *
- * Declares initial configuration for simplenews_demo.
+ *Declares initial configuration for simplenews_demo.
+ 
  */

That's not only wrong, but it ADDS two coding standards violations. I stopped reviewing at that point. Leaving this at NW for that reason.

I don't know what the point of the patch in #24 is. A patch like this is unreviewable because it's so large and changes so many things, and is unmanageable because it's trying to do everything for the entire project rather than being constrained to one type of problem or fixing things in one place.

IMO at this point the patch should only try fix short array syntax. That will account for a large majority of the coding standards violations. The patch in #18 fits that description but it needs to be rerolled now that #3055728: Convert from Simpletests to PHPUnit tests and #2994827: Simplenews Drush 9 support are committed. That would be easily reviewable and would fix a huge number of coding standards problems and allow the others to be more easily addressed. I will be happy to review a re-roll of #18.

As far as the comment in #21:

I will allow #3031011: Add rules support a chance to go first because it is very close.

Thank you but I would prefer this current issue gets committed first since it will actually help me to finish the Rules support. In #3031011: Add rules support I mentioned that I need to use the code from SimplenewsTestBase and SimplenewsSubscribeTest in order to write functional tests for the Rules integration. It would be nice if those were free of things like long array syntax before I copied code out of them, otherwise I will just have to fix ~100 coding standards violations in my copy as part of #3031011: Add rules support. I don't like to duplicate work ... I'm perfectly happy delaying the functional tests for #3031011: Add rules support until all the array syntax in simplenews is fixed.

OK. Rerolling just #18, as indeed something seems wrong in running one of those sniffs in separate from the others that introduces additional coding standards violations.

As before, this is the result of running drupalcbf --sniffs=Drupal.Arrays.DisallowLongArraySyntax . in the current codebase.

@AdamPS, in reply to #27, yes I can easily try to do the rest.

Setting to RTBC, as the tests passed. This will leave as 2nd highest problem the missing method scopes in classes, something which seems to be partially addressed in #3079894: Fix visibility of test methods.

The changes in #3075393: PHP 7.2 error for SpoolStorage addIssue do not affect the output of the patch, so #28 is still valid.

     14 Drupal.Commenting.DocComment.ShortFullStop
     14 Squiz.ControlStructures.SwitchDeclaration.SpacingAfterBreak
     15 Drupal.Classes.UnusedUseStatement.UnusedUse
     17 Drupal.Arrays.Array.ArrayIndentation
     18 Drupal.Commenting.InlineComment.SpacingAfter
     23 Drupal.Commenting.FunctionComment.Missing
     27 Drupal.Arrays.Array.CommaLastItem
     31 Drupal.Commenting.InlineComment.InvalidEndChar
     33 Squiz.WhiteSpace.FunctionSpacing.AfterLast
     34 Drupal.Classes.ClassDeclaration.CloseBraceAfterBody
     42 Drupal.WhiteSpace.ScopeIndent.IncorrectExact
    102 Drupal.Scope.MethodScope.Missing

The patch in #28 applies, and removes all usage of array(). I did a visual check of the patch to make sure there were no code alterations other than array() - again it looks fine.

are you able to make a fairly firm commitment to produce patches and review

Yes, with the caveat that I'm out of town and mostly off-grid this coming week so I won't be able to do much until I get back.

missing method scopes in classes, something which seems to be partially addressed in #3079894: Fix visibility of test methods

We can widen that if you want.

Here.

There is only one real difference between #28 and this one, but I couldn't generate an interdiff.

> -  protected function setupSubscriptionBlock($settings = array()) {
> +  protected function setupSubscriptionBlock($settings = []) {

Also, updating the top 12 outstanding issues, shows the impact of #3079894: Fix visibility of test methods:

     14 Drupal.Commenting.DocComment.ShortFullStop
     14 Squiz.ControlStructures.SwitchDeclaration.SpacingAfterBreak
     15 Drupal.Classes.UnusedUseStatement.UnusedUse
     17 Drupal.Arrays.Array.ArrayIndentation
     18 Drupal.Commenting.InlineComment.SpacingAfter
     23 Drupal.Commenting.FunctionComment.Missing
     27 Drupal.Arrays.Array.CommaLastItem
     31 Drupal.Commenting.InlineComment.InvalidEndChar
     33 Squiz.WhiteSpace.FunctionSpacing.AfterLast
     34 Drupal.Classes.ClassDeclaration.CloseBraceAfterBody
     42 Drupal.WhiteSpace.ScopeIndent.IncorrectExact
     59 Drupal.Scope.MethodScope.Missing

Tests passed. Setting back to RTBC.

OK. Here's the patch.

I'm uploading two files. The txt are the manual changes I did to the code before executing drupalcbf . in the current codebase. This file is 3607 lines long. Please ignore the .gitignore file at the start of this file.

The .patch file is the aggregated output of both steps. This one is 5569 lines long.

I chose the wrong placeholder type, and I missed one standard violation.

Now with interdiff.

Found a few PHP fatal errors when running drupal-check that are related to now having type hints in interface function declarations that must be reflected in the classes implementing them.

FWIW, I would definitely recommend to do one type of problem per issue, it's much easier to review, especially with a word-based git diff. You can scan quickly through a long file and ensure that nothing unexpected shows up. Consider that a comment for next time, too late to split up now.

+++ b/src/Spool/SpoolStorage.php
@@ -152,7 +152,7 @@
    * {@inheritdoc}
    */
-  public function updateMails($msids, array $data) {
+  public function updateMails(array $msids, array $data) {
     $this->connection->update('simplenews_mail_spool')
       ->condition('msid', (array) $msids, 'IN')
       ->fields([
diff -u b/src/Subscription/SubscriptionManager.php b/src/Subscription/SubscriptionManager.php

It's a new branch that's not stable yet, so I guess fine, but the array type hint is a problematic one because it can easily result in API changes when done on interfaces/base classes as the interdiff here shows perfectly.

1. It can't be because it has the same name as the current class. What you'd need to do use use X as CoreX. That's why it using the full namespace there.

3. Just remove t(). t() was used on anything that looked like a translatable string in the past, but this is pointless, we know it's not going to be translated, the test is EN and we know exactly what's there. Even if languages are enabled, only explicitly added translations exist in a test. In fact, most t() in tests should just be dropped instead of changing to $this->t() IMHO, but the work is done now I suppose..

5. Yeah, not sure if this used the default Drupal coding standard or the one that core is using. AFAIK the things that aren't used by core might not actually have been agreed on. As far as I know, top level casses shouldn't be used but referenced as \, that's definitely what's done with \Drupal (there also explicitly in non-namespaced files, for consistency).

PS: Having comments above the code snippet confused me about 3x while reading the comment and going up/down ;)

Replying to #46 also:

1. As Berdir stated, we can't really do that use in there.. This class has the same name as the one in core - both are called MailFormatHelper, so we can't do 'use'. The only way is to ignore the coding standard on that one line, so that it stops being raised as a problem. Either that or we do the 'use X as coreX', but that is honestly worse than ignoring it.

2. That first line needs to be less than 80 chars. We can set it to "Base for Recipient Handlers" instead of "Base for Recipient Handler classes.". But that changes nothing. I'd just leave it as is for the moment.

3. Yes, we can change the contents of the t() to remove the warning. We could maybe also remove the t() altogether, as suggested by @Berdir. And no, I think I didn't change this rule in any of the /tests/. But I consider removing the t() in tests to be out of the scope of this issue. For now, I'd leave the t() and the ignores there. The new issue should make sure to remove the ignores.

4. We can transform that @todo into a multi-line comment so that the automated process doesn't remove that line.

5. That one was probably added by PhpStorm when I messed around with the @throws comment below. The issue is in this line below: throw new Exception("Recipient handler requires a single newsletter ID."). I think if the exception was created by doing throw new \Exception, there would be no auto-correction of the issue. As it is, I actually prefer it this way, as it is treating the base Exception class the same way as the Drupal classes. But we can also remove the use line and change the throw line.

My point of view, there are only minor corrections needed for 4 & 5. If you agree, I can create the new patch addressing those.

As to the ammount of @CodingStandardsIgnore lines:

There are 6 blocks of commented-out code, one of which is because of #3062330: Temporarily remove simplenews_scheduler from demo as it's broken on 8.x-2.x. The other 5, we should probably investigate their need to stay commented or simply delete them. Again, deleting them (and the surrounding ignores) is something that I consider out of scope of this issue.

There are also 3 @CodingStandardsIgnoreLine in modules/simplenews_demo/src/Tests/SimplenewsDemoTest.php because of the same issue. I didn't add a comment on those, but we can do that if it helps.

The remaining 6 @CodingStandardsIgnoreLine are:

• src/Mail/MailFormatHelper.php; Discussed in point 1 above.
• tests/src/Functional/SimplenewsAdministrationTest.php (3x): Discussed in point 3 above.
• tests/src/Functional/SimplenewsI18nTest.php: commented out call to simplenews_cron() - maybe delete the line instead? Or at least add a @todo if this should be left there.
• tests/src/Functional/SimplenewsSubscribeTest.php; commented out call to file_put_contents() - seems like a debug leftover - I'd suggest to delete the line - but not sure if this would be in scope.

Fixes as per #52

It seems drupalcs missed one that is identified by the tests here in d.o (line 12 of src/Form/SubscriptionsBlockForm.php), and maybe I can do something about those 3 lines in the README.txt still.

In any case, I've added #3080777: Remove t() from tests and #3080781: Investigate and remove dead code to address the points still left from #52.

OK. This fixes the coding standard in line 12, and in addition it provides proper docs for the setUniqueId() function which ends up not using inheritance from anywhere, making the inheritdoc comment wrong.

The 3 errors in the README.txt, are a bit deeper. Most of that file was written 7-9 years ago, and it needs a major revamp at the moment.

Thanks! And the tests even passed this time. :)