Code sniffer fixes taken from https://www.drupal.org/pift-ci-job/1443970
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3089551-13.patch | 120.34 KB | tmaiochi |
| #8 | 3089551-8.patch | 102.36 KB | idebr |
Code sniffer fixes taken from https://www.drupal.org/pift-ci-job/1443970
| Comment | File | Size | Author |
|---|---|---|---|
| #13 | 3089551-13.patch | 120.34 KB | tmaiochi |
| #8 | 3089551-8.patch | 102.36 KB | idebr |
Comments
Comment #2
berdirSomething with that patch is very wrong as it tries to apply this all to anon-existing PHP_CodeSniffer.
If you use patch -p1 then it asks for each file and you can just copy & paste the original filename each time, kinda tedious but works..
Comment #3
berdirLooked at this to commit it before #3090356: Convert tests to phpunit but I might do that now and then it probably makes sense to do this with a new patch after that was committed.
Comment #4
naheemsays commentedTaken from https://dispatcher.drupalci.org/job/drupal8_contrib_patches/9696/artifac... then modified to apply.
Comment #5
idebr commentedThe testbot reports 20 coding standards issues: https://www.drupal.org/pift-ci-job/1464741
Comment #6
idebr commentedAttached patch fixes the remaining code style errors.
I was unable to generate a meaningful interdiff because of the many changes in HEAD.
Comment #7
naheemsays commentedpatch didnt apply, needs re-rolling
Comment #8
idebr commentedRerolled #6 against 8.x-1.x
Comment #9
megachrizNot really a code style, but a textual error. A poll is no feed, so I think the word 'feed' should be replaced with 'poll'. Or should this be an other issue?
'feed' should be 'poll' (see also #1).
Since we're in a class, we could use
$this->t()here, though I'm not sure if this is required for the coding standards.Should be "Form submit handler to return back to the poll view.". You don't return to the "submit function".
Is it correct that this doesn't need to wrap at 80 chars? Maybe it is, I'm not sure.
I understand that removing commented out code is needed in order to comply to the coding standards, but in this case it does make the parameter
$previewof the methodpollCreate()useless. Shouldn't instead a@todobe added here, ideally one that links to an issue that explains why this is commented out?This line can probably be removed completely. Not only it refers to a no longer existing property, the method
getUserName()only returns data and I think it adds no value to this test.Just as noted elsewhere, this may deserve a
@todoto explain why this code is commented out.Comment #10
saphemmy commentedComment #11
saphemmy commentedComment #12
tmaiochi commentedI'll work on this!
Comment #13
tmaiochi commentedI made a reroll from patch #8 and I fixed the new PHPCS messages, and only dependency injection messages are left, as the issue is just for code style I left them like this!
Kindly review it!
Comment #15
tmaiochi commentedLet's me fix this error!
Comment #16
tmaiochi commentedI was testing the module with drupal 8.9 and this error didn't happen, so I tried to test with Drupal 9.2 and the test passed too, as you can see on comment #13 I added a test with drupal 9.2 and it passed.
Then I realized that in drupal 9.3 when you click on the permissions on the
admin/modulespage, you are redirected toadmin/people/permissions/module/module_mymodule, however in previous versions of drupal when you click on permission you were redirected toadmin/people/permissions#module-mymodule.So in my opinion it is necessary to open an issue to discuss how to fix this test, as this issue is only for code standards.
Comment #17
tmaiochi commentedI created a issue for fix this test that give the error.
#3270106: Fix errors in PollFieldUITest on Drupal 9.3
Comment #18
sutharsan commentedLet's get this in. Patches like this that touch everything should not be discussed too long. If it does not break stuff, we can fix minor points later.
Comment #19
bramdriesenI'm planning to incorporate this ticket into #3417969: Adopt GitLab CI template so we can fix all those things in one single merge request.
Updating issue credits.
Comment #20
bramdriesenManually applied the patch file and fixed the hunks which could not apply after #2877966: Coding standards: use short array syntax got added.
Latest test run highlights some more fixes which are needed. Will fix them on the parent MR.
https://git.drupalcode.org/project/poll/-/jobs/715342
Comment #21
bramdriesenPHPCS fixes resolved: https://git.drupalcode.org/project/poll/-/jobs/715890
Comment #22
bramdriesenComment #23
bramdriesen