Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Currently there are over 1000 warnings, including many for array syntax array() should be [].
We should fix those that we easily can, preferably automatically - e.g. we could try to get help from the auto-generated coding standards patch that Drupal.org supplies on a failed text.
Comment | File | Size | Author |
---|---|---|---|
#58 | simplenews-code-standards-3037140-58.patch | 704 bytes | jcnventura |
| |||
#53 | simplenews-code-standards-3037140-53.patch | 215.55 KB | jcnventura |
| |||
#53 | simplenews-code-standards-3037140-53.txt | 151.5 KB | jcnventura |
#53 | interdiff_43_53.txt | 4.3 KB | jcnventura |
#35 | simplenews-code-standards-3037140-35.patch | 289.92 KB | jcnventura |
|
Comments
Comment #2
neeraprajapati CreditAttribution: neeraprajapati at Valuebound commentedComment #3
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@neeraprajapati Thanks for offering to help. As it says in the issue summary, please can you wait until around the end of the year? I will set the status back to active when it's a good time. The problem is that if we commit this issue, then every other patch is likely to need a reroll - and at the moment there are a lot of issues with patches waiting.
Comment #4
TR CreditAttribution: TR commentedHow 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.
Comment #5
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedI'm fairly sure there are issues in needs work or needs review state that alter tests. There would also be disruption to anyone (including me!) with work in a cloned repository that hasn't yet been uploaded as a patch.
I'm not attracted to that as it seems like it will make two separate occasions when we cause disruption.
Please can you explain how it would help with that or any other reason why there is any urgency to fix this?
Comment #6
TR CreditAttribution: TR commentedMy 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.
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.
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.
Comment #7
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@TR Many thanks for the offer of a patch - I really do appreciate it and I will let you know as soon as I am ready to commit it.
I agree that it will be better to have the coding standards fixed. However the problem of disruption is very real for me as I have various local cloned repositories with work in progress. Also for example #3042666: Drupal 9 Deprecated Code Report has a patch that changes tests, and I find that all the deprecation warnings are even more disruptive than the coding standards warnings.
We will get a D9 version of simplenews eventually, but the top priority is to get a stable and usable D8 version. I have raised #3055728: Convert from Simpletests to PHPUnit tests and yes I am aware that this needs fixing before D9.
Comment #8
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@TR I hope to commit the last of my current issues on Tuesday (9th July). If you are still offering, that would be a great time to fix the coding standards. I will update the status on this issue when I have finished.
I am in favor of fixing the whole module: code and tests because then there is only disruption once. Of course there are issues with patches, but most of them have not been active for some time.
If I don't hear from you by 20th July then I will next offer the option to another developer to work on #3055728: Convert from Simpletests to PHPUnit tests which is of course similar and disruptive to many files.
Comment #9
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThis issue is ready for a patch. If TR or neeraprajapati or anyone else is willing to do it then please assign yourself. I removed the current assignment so that it's clear when there is a new one.
Comment #10
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedOK, giving priority to #3055728 which is a D9 blocker.
Comment #11
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedThere 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.
Comment #13
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedGreat many thanks. The multiple stages sounds like a good idea. Please can you provide a link to a page that confirms your command is the correct one for the current Drupal coding standards?
Comment #14
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedhttps://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.
Comment #15
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks for explanation in #14.
NB as per #10 I will check in #3055728: Convert from Simpletests to PHPUnit tests first plus also your other patch #2994827: Simplenews Drush 9 support. You are welcome to continue to investigate this issue now but note that any patch here will need a reroll.
Comment #16
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedNo 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.
Comment #17
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedIt's not obvious to my why the tests failed. However as so many tests are failing then I expect it should be easy to find - not a subtle bug. I would apply the patch to a test site and run through a simple scenario by hand. Even just sending a test newsletter looks like it should be enough (SimplenewsAdministrationTest around line 814).
Comment #18
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedLet'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 .
Comment #20
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedSetting 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.
Comment #21
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedGreat thanks @jcnventura
Yes I will commit following the sequence as discussed in #16 - we need to commit other issues first so as not to force them to do unpleasant re-rolls. I will allow #3031011: Add rules support a chance to go first because it is very close.
I'll let you know at the right time, then you can rerun your job and then I will commit.
Comment #22
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedTotally 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.
Comment #23
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks for the link. I prefer if possible to have a second developer involved and would only commit my own patch if I really can't find anyone to work with so please do stay involved if you can.
Comment #24
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedOK. 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 .
Comment #26
TR CreditAttribution: TR commentedNote 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:
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:
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.
Comment #27
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks @TR
I have queued a retest.
We already have #3037704: Fix intermittent test failures
True. Looks like that is because the file has /* instead of /** and the tool tries to correct that but does it wrongly.
I can certainly see advantages to that but I am also attracted by the idea of getting it done in one go. Every time we commit a huge coding standards patch it likely forces re-rolls on other issues - and there are some active ones at the moment. So if we split it into multiple widespread disruptive patches then I think we need to complete them all within 2-3 weeks maximum. How about if we aim for two stages: the short arrays then everything else that can be fixed automatically? @jcnventura, @TR are you able to make a fairly firm commitment to produce patches and review?
OK great, that sounds good to me thank you.
Comment #28
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedOK. 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.Comment #29
jcnventura CreditAttribution: jcnventura at 1xINTERNET commented@AdamPS, in reply to #27, yes I can easily try to do the rest.
Comment #30
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedGreat thanks.
Sorry I just committed #3075393: PHP 7.2 error for SpoolStorage addIssue, but it's only a one-line change so we might be lucky and it won't break. I won't make any other commits from now on.
Comment #31
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedSetting 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.
Comment #32
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedLooks good to me. I will wait 1 day for a second opinion from @TR or anyone else then I will commit.
Comment #33
TR CreditAttribution: TR commentedThe 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.
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.
We can widen that if you want.
Comment #34
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@jcnventura sorry please can you reroll one more time then I will definitely commit. I have the idea that you can do this very fast by just running the automatic job, so I decided the commit the other issue first.
Comment #35
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedHere.
There is only one real difference between #28 and this one, but I couldn't generate an interdiff.
Also, updating the top 12 outstanding issues, shows the impact of #3079894: Fix visibility of test methods:
Comment #36
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedTests passed. Setting back to RTBC.
Comment #37
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedComment #39
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedExcellent. Now ready for you to work on the remaining rules.
Comment #40
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedOK. 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.
Comment #42
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedI chose the wrong placeholder type, and I missed one standard violation.
Now with interdiff.
Comment #43
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedFound 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.
Comment #45
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedComment #46
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedGreat many thanks for your work on this one which deserves great credit as it must have been quite boring! Good news that this also seems to solve #2856182: $this->t() should be used instead of t() for Drupal 8 version..
I reviewed your txt file. I wonder if next time round you could submit it alone as a patch file, then I will do each stage as a separate commit. Yes I realise I had said last time do it as one, but I had imagined to do all the automatic changes as one. I think it adds some value to anyone tracing back to be able to distinguish the manual edits from the automatic ones.
One general point is that I would prefer to cut down @codingStandardsIgnore. If possible we could find a way to alter to code so there is no warning. Or if we have to have it, it would be useful to indicate why unless it's obvious. Quite a few of these are related to commented out code. In many of these there is no comment to indicate why the code is present but commented out so it's not necessarily performing any useful purpose. (Exception is the one that says
// Disabled because of issue #3062330.
). Perhaps we should just delete the commented out code - although we maybe ought to check with @Berdir first. I would be happy to defer these to a separate issue and you can leave a warning in for now. I'm less happy to add @codingStandardsIgnore because that seems to be heading in the wrong direction.Specific comments:
1. Looks like the line below should not be ignored - there should be a use
\Drupal\Core\Mail\MailFormatHelper
2. New comment says "Base for Recipient Handler classes" but this is not a base for all recipient handler classes, only a subset. How about something like "Base for Recipient Handlers that access the database directly using Select".
3. At a guess this warning is because the translated string (safe) is being joined to something (unsafe) - which is generally wrong. Any idea why we can just fix the code to avoid the warning? I.e. like the following in UserRegistrationTest
$this->assertTitle(t('Set password | Drupal'), 'Page title is "Set password".');
4. This isn't really helping - the @todo is not related to the following comment.
5. I've not generally seen this in code - can you explain why?
Comment #47
BerdirFWIW, 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.
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 ;)
Comment #48
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@Berdir thanks for the review and various useful comments for us to learn from and avoid future mistakes.
Ah yes - let's do that then, it's a fairly common pattern in core.
Yes after considering it overnight, I was independently thinking the same. I reckon it ought to be possible quite fast with an editor macro perl script or the like. However I'm not going to insist.
Comment #49
AdamPS CreditAttribution: AdamPS at AlbanyWeb commented@Berdir, would also appreciate your response to this which was a general comment before the individual points:
Quite a few of these are related to commented out code. In many of these there is no comment to indicate why the code is present but commented out so it's not necessarily performing any useful purpose. (Exception is the one that says // Disabled because of issue #3062330.). Perhaps we should just delete the commented out code - although we maybe ought to check with @Berdir first
Comment #50
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedReplying 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.
Comment #51
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedAs 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:
Comment #52
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks @jcnventura. Yes you are right lets defer bigger tasks to separate issues: remove t() from tests; resolve commented out code.
However my 5 specific comments are simple to solve and so I think worth doing. I am happy to commit once they are done.
1.
use XX as YYXX
occurs 291 times in core so I think that's exactly what this warning is telling us to write2. My suggestion is 75 characters: "Base for Recipient Handlers that access the database directly using Select".
3. I prefer Berdir's idea: remove t() entirely in these 3 cases only (leave it for all other cases)
4. Good idea, use multiline comment as you say
5.
throw new \Exception
occurs 92 times in core so I think that's the correct solutionComment #53
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedFixes as per #52
Comment #55
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedExcellent - that's made things a lot easier for all devs, many thanks @jcnventura
Comment #56
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedIt 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.
Comment #57
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedGreat thanks
Comment #58
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedOK. 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.
Comment #60
AdamPS CreditAttribution: AdamPS at AlbanyWeb commentedThanks done. We have a separate issue for the README so I'll set this one back to fixed.
Comment #61
jcnventura CreditAttribution: jcnventura at 1xINTERNET commentedThanks! And the tests even passed this time. :)