Comments

nbz created an issue. See original summary.

berdir’s picture

Status: Needs review » Needs work

Something 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..

berdir’s picture

Looked 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.

naheemsays’s picture

Status: Needs work » Needs review
StatusFileSize
new38.93 KB
idebr’s picture

Status: Needs review » Needs work

The testbot reports 20 coding standards issues: https://www.drupal.org/pift-ci-job/1464741

tests/src/Functional/PollBlockTest.php
line 19 Doc comment is empty
tests/src/Functional/PollCreateTest.php
92 Line exceeds 80 characters; contains 85 characters
tests/src/Functional/PollFieldUITest.php
39 Public method name "PollFieldUITest::testPollFieldUI" is not in lowerCamel format
tests/src/Functional/PollTestBase.php
14 Missing short description in doc comment
17 Class property $admin_user should use lowerCamel naming without underscores
19 Missing short description in doc comment
22 Class property $web_user should use lowerCamel naming without underscores
24 Missing short description in doc comment
78 Line exceeds 80 characters; contains 82 characters
86 Type hint "\Drupal\user\UserInterface" missing for $account
110 Comment indentation error, expected only 1 spaces
112 Line exceeds 80 characters; contains 84 characters
tests/src/Functional/PollVoteCheckHostnameTest.php
8 Line exceeds 80 characters; contains 85 characters
14 Doc comment is empty
tests/src/Functional/PollVoteTest.php
34 Line exceeds 80 characters; contains 234 characters
34 Comment indentation error, expected only 1 spaces
39 Comment indentation error, expected only 1 spaces
48 Comment indentation error, expected only 1 spaces
50 Comment indentation error, expected only 1 spaces
54 Comment indentation error, expected only 1 spaces

idebr’s picture

Status: Needs work » Needs review
StatusFileSize
new101.88 KB

Attached patch fixes the remaining code style errors.

I was unable to generate a meaningful interdiff because of the many changes in HEAD.

naheemsays’s picture

Status: Needs review » Needs work

patch didnt apply, needs re-rolling

idebr’s picture

Status: Needs work » Needs review
StatusFileSize
new102.36 KB

Rerolled #6 against 8.x-1.x

megachriz’s picture

Status: Needs review » Needs work
  1. +++ b/src/Form/PollForm.php
    @@ -46,7 +46,7 @@ class PollForm extends ContentEntityForm {
    -        $form_state->setErrorByName('question', $this->t('A feed named %feed already exists. Enter a unique question.', array('%feed' => $poll->label())));
    +        $form_state->setErrorByName('question', $this->t('A feed named %feed already exists. Enter a unique question.', ['%feed' => $poll->label()]));
    

    Not 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?

  2. +++ b/src/Form/PollItemsDeleteForm.php
    @@ -15,7 +15,7 @@ class PollItemsDeleteForm extends ContentEntityConfirmFormBase {
    -    return $this->t('Are you sure you want to delete all items from the feed %feed?', array('%feed' => $this->entity->label()));
    +    return $this->t('Are you sure you want to delete all items from the feed %feed?', ['%feed' => $this->entity->label()]);
    

    'feed' should be 'poll' (see also #1).

  3. +++ b/src/Form/PollViewForm.php
    @@ -243,30 +258,30 @@ class PollViewForm extends FormBase implements BaseFormIdInterface {
    -        '#display_value' => t('@percentage%', array('@percentage' => $percentage)) . $display_votes,
    +        '#display_value' => t('@percentage%', ['@percentage' => $percentage]) . $display_votes,
    

    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.

  4. +++ b/src/Form/PollViewForm.php
    @@ -320,10 +330,7 @@ class PollViewForm extends FormBase implements BaseFormIdInterface {
    -   * Back to poll view submit function.
    -   *
    -   * @param array $form
    -   * @param \Drupal\Core\Form\FormStateInterface $form_state
    +   * Form submit handler to return back to poll view submit function.
    

    Should be "Form submit handler to return back to the poll view.". You don't return to the "submit function".

  5. +++ b/src/PollStorageInterface.php
    @@ -14,27 +14,26 @@ interface PollStorageInterface extends EntityStorageInterface {
    +   * @deprecated in poll:8.x-1.0 and is removed from poll:8.x-2.0. Use \Drupal\poll\PollVoteStorageInterface::saveVote() instead.
    ...
    +   * @deprecated in poll:8.x-1.0 and is removed from poll:8.x-2.0. Use \Drupal\poll\PollVoteStorageInterface::cancelVote() instead.
    
    @@ -42,13 +41,15 @@ interface PollStorageInterface extends EntityStorageInterface {
    +   * @deprecated in poll:8.x-1.0 and is removed from poll:8.x-2.0. Use \Drupal\poll\PollVoteStorageInterface::getTotalVotes() instead.
    
    @@ -56,27 +57,28 @@ interface PollStorageInterface extends EntityStorageInterface {
    +   * @deprecated in poll:8.x-1.0 and is removed from poll:8.x-2.0. Use \Drupal\poll\PollVoteStorageInterface::getVotes() instead.
    ...
    +   * @deprecated in poll:8.x-1.0 and is removed from poll:8.x-2.0. Use \Drupal\poll\PollVoteStorageInterface::deleteVotes() instead.
    
    @@ -84,13 +86,15 @@ interface PollStorageInterface extends EntityStorageInterface {
    +   * @deprecated in poll:8.x-1.0 and is removed from poll:8.x-2.0. Use \Drupal\poll\PollVoteStorageInterface::getUserVote() instead.
    

    Is it correct that this doesn't need to wrap at 80 chars? Maybe it is, I'm not sure.

  6. +++ b/tests/src/Functional/PollTestBase.php
    @@ -112,20 +119,14 @@ abstract class PollTestBase extends BrowserTestBase {
    -//    if ($preview) {
    -//      $this->drupalPostForm('poll/add', $edit, t('Preview'));
    -//      $this->assertPollChoiceOrder($choices, $index, TRUE);
    -//      list($edit, $index) = $this->pollGenerateEdit($title, $choices, $index);
    -//    }
    

    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 $preview of the method pollCreate() useless. Shouldn't instead a @todo be added here, ideally one that links to an issue that explains why this is commented out?

  7. +++ b/tests/src/Functional/PollVoteCheckHostnameTest.php
    @@ -31,26 +33,26 @@ class PollVoteCheckHostnameTest extends PollTestBase {
    -    //  $this->web_user->getUserName();
    +    // $this->web_user->getUserName();
    

    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.

  8. +++ b/tests/src/Functional/PollVoteTest.php
    @@ -17,49 +16,25 @@ class PollVoteTest extends PollTestBase {
    -//    $this->drupalGet('poll/' . $this->poll->id() . '/votes');
    -//    $this->assertText(t('This table lists all the recorded votes for this poll. If anonymous users are allowed to vote, they will be identified by the IP address of the computer they used when they voted.'), 'Vote table text.');
    -//    $options = $this->poll->getOptions();
    -//    debug($options);
    -
    -   // $this->assertText($this->poll->getOptions()[0], 'Vote recorded');
    -
    -    // Ensure poll listing page has correct number of votes.
    -//    $this->drupalGet('poll');
    -//    $this->assertText($this->poll->label(), 'Poll appears in poll list.');
    -//    $this->assertText('1 vote', 'Poll has 1 vote.');
    ...
    -//    $this->drupalGet('poll/' . $this->poll->id() . '/votes');
    -//    $this->assertNoText($choices[0], 'Vote cancelled');
    -
    -    // Ensure poll listing page has correct number of votes.
    -//    $this->drupalGet('poll');
    -//    $this->assertText($title, 'Poll appears in poll list.');
    -//    $this->assertText('0 votes', 'Poll has 0 votes.');
    -
    -    // Log in as a user who can only vote on polls.
    -//    $this->drupalLogout();
    -//    $this->drupalLogin($restricted_vote_user);
    

    Just as noted elsewhere, this may deserve a @todo to explain why this code is commented out.

saphemmy’s picture

Assigned: Unassigned » saphemmy
saphemmy’s picture

Assigned: saphemmy » Unassigned
tmaiochi’s picture

Assigned: Unassigned » tmaiochi

I'll work on this!

tmaiochi’s picture

Assigned: tmaiochi » Unassigned
Status: Needs work » Needs review
StatusFileSize
new120.34 KB

I 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!

Status: Needs review » Needs work

The last submitted patch, 13: 3089551-13.patch, failed testing. View results

tmaiochi’s picture

Assigned: Unassigned » tmaiochi

Let's me fix this error!

tmaiochi’s picture

Assigned: tmaiochi » Unassigned
Status: Needs work » Needs review

I 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/modules page, you are redirected to admin/people/permissions/module/module_mymodule, however in previous versions of drupal when you click on permission you were redirected to admin/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.

tmaiochi’s picture

I created a issue for fix this test that give the error.
#3270106: Fix errors in PollFieldUITest on Drupal 9.3

sutharsan’s picture

Status: Needs review » Reviewed & tested by the community

Let'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.

bramdriesen’s picture

Assigned: Unassigned » bramdriesen
Parent issue: » #3417969: Adopt GitLab CI template

I'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.

bramdriesen’s picture

Manually 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

bramdriesen’s picture

bramdriesen’s picture

Status: Reviewed & tested by the community » Fixed
bramdriesen’s picture

Assigned: bramdriesen » Unassigned

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.