Fix the issues reported by phpcs

The report shows errors/warnings for two files, but the MR changes four files.

@apaderno,

If you see the reporter command,
vendor/bin/phpcs --standard="Drupal,DrupalPractice" -n --extensions="php,module,inc,install,test,profile,theme", There is 'yml' is missing in extensions list, that's why they unable to find issue in 'tests/verf_test_views/verf_test_views.info.yml' and 'verf.info.yml'

I have run this command
vendor/bin/phpcs --standard="Drupal,DrupalPractice" -n --extensions="php,module,inc,install,test,profile,theme,yml"
and I got issues in 4 files that I have fixed and created MR.

@Nishant In that case, the command shown in the issue summary must be updated to include that extension. The command shown in the issue summary must be the same used when fixing the existing issues.

updated the issue @apaderno

Will take care from next. @apaderno

Hi I reviewed MR!9, and there is still remaining issue reported by phpcs shown below:

FILE: .../studenttrainees/Drupal.org/drupalorg-site/docroot/modules/contrib/verf/tests/src/Functional/IntegrationTest.php
----------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------------------
 171 | WARNING | Unused variable $referencingPublished.
 175 | WARNING | Unused variable $referencingUnpublished.
----------------------------------------------------------------------------------------------------------------------


FILE: ...udenttrainees/Drupal.org/drupalorg-site/docroot/modules/contrib/verf/src/Plugin/views/filter/EntityReference.php
----------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
----------------------------------------------------------------------------------------------------------------------
   7 | WARNING | [x] Unused use statement
 136 | WARNING | [ ] t() calls should be avoided in classes, use
     |         |     \Drupal\Core\StringTranslation\StringTranslationTrait and $this->t() instead
----------------------------------------------------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------------------------------------------------------

Time: 259ms; Memory: 10MB

I will try to work on this.

Created a patch to fix the remaining issues.

Please review.
Thank You.

Hi I reviewed the patch #13 it applied cleanly. No errors/warnings by phpcs are reported after applying the patch.

Thanks!

Hi @paraderojether, your patch applied cleanly, but I found 1 error. Here are the steps I have followed:

1. Taken clone from git version 2.0.x in drupal 10.1.2
2. Applied your patch and ran the phpcs command
3. Found 1 error

4. Then ran the phpcbf command to fix that error. Now It is error free.

Attaching screenshots and patch.
Please review.

Hi @Abh1shek Chauhan, I have applied your patch.

These are the steps I followed:

1. Took clone from git version 2.0.x in drupal 10.1.x

2. Ran this command:

./vendor/bin/phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml,twig modules/contrib/verf

3. Applied patch and again ran phpcs command.

I found no error. Moving it to RTBC.

     $form['show_unpublished'] = [
       '#type' => 'checkbox',
-      '#title' => t("Ignore access control"),
-      '#default_value' => $this->options['show_unpublished'],
+      '#title' => $this->t("Ignore access control"),
+      '#default_value' => $options['show_unpublished'],
     ];

There is not any $options variable in buildOptionsForm().

  /**
   * {@inheritdoc}
   */
  public function buildOptionsForm(&$form, FormStateInterface $form_state) {
    parent::buildOptionsForm($form, $form_state);

    $form['show_unpublished'] = [
      '#type' => 'checkbox',
      '#title' => t("Ignore access control"),
      '#default_value' => $this->options['show_unpublished'],
    ];

    if (!$this->targetEntityType->hasKey('bundle')) {
      return $form;
    }

    $options = [];
    $bundleInfo = $this->entityTypeBundleInfo->getBundleInfo($this->targetEntityType->id());
    foreach ($bundleInfo as $id => $info) {
      $options[$id] = $info['label'];
    }

    $form['verf_target_bundles'] = [
      '#type' => 'checkboxes',
      '#title' => $this->t('Target entity bundles to filter by'),
      '#options' => $options,
      '#default_value' => array_filter($this->options['verf_target_bundles']),
    ];

    return $form;
  }

    * @return \Drupal\Core\Entity\EntityInterface[]
+   *   The Reference entity.

Reference is misspelled, since it is not the first word.

    * @param string $locator
-   *   input id, name or label
+   *   Input id, name or label.

It is ID, not id.

    * @param string $value
-   *   option value
+   *   Option value.

A definite article is missing.

-    $referencingPublished = $this->drupalCreateNode([
+    $this->drupalCreateNode([
       'type' => $this->nodeType->id(),
       'field_refs' => [['target_id' => $published->id()]],
     ]);
-    $referencingUnpublished = $this->drupalCreateNode([
-      'type' => $this->nodeType->id(),
-      'field_refs' => [['target_id' => $unpublished->id()]],
-    ]);

The code is creating two nodes. Why is the code that creates the node with a reference to the unpublished node removed?

Here is the updated patch as per @apaderno comments.

-    $form['show_unpublished'] = [
-      '#type' => 'checkbox',
-      '#title' => t("Ignore access control"),
-      '#default_value' => $this->options['show_unpublished'],
-    ];
-
     if (!$this->targetEntityType->hasKey('bundle')) {
       return $form;
     }
@@ -148,6 +140,12 @@ class EntityReference extends InOperator implements ContainerFactoryPluginInterf
       $options[$id] = $info['label'];
     }
 
+    $form['show_unpublished'] = [
+      '#type' => 'checkbox',
+      '#title' => $this->t("Ignore access control"),
+      '#default_value' => $options['show_unpublished'],
+    ];
+

The existing code correctly uses $this->options. It does not need to be changed, except to remove the call to t(), nor those lines need to be moved.

    * @return \Drupal\Core\Entity\EntityInterface[]
+   *   The reference entity.

Since the return value is an array, it cannot be described as The reference entity.

-   *   input id, name or label
+   *   Input id, name or label.

It is ID, not id.
A comma is missing after name.

-    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $published->getTitle());
-    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $unpublished->getTitle());
+    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $referencingPublished->getTitle());
+    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $referencingUnpublished->getTitle());
 
     $this->drupalLogin($this->author);
     $this->drupalGet('verftest');
-    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $published->getTitle());
-    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $unpublished->getTitle());
+    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $referencingPublished->getTitle());
+    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $referencingUnpublished->getTitle());
 
     $this->drupalLogout();
     $this->drupalGet('verftest');
-    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $published->getTitle());
+    $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', $referencingPublished->getTitle());
     $this->assertSelectOptionCanBeSelected('Refs (VERF selector)', '- Restricted access -');
-    $this->assertSelectOptionCanNotBeSelected('Refs (VERF selector)', $unpublished->getTitle());
+    $this->assertSelectOptionCanNotBeSelected('Refs (VERF selector)', $referencingUnpublished->getTitle());

Why are the variables passed to those methods changed?

Updated patch as per #20 comment.

Tests that runs when a patch is checked are failing.

please review this, I have resolved some issues.

Hello, I tried applying the patch provided in #25, it is applying succesfully.
I ran the command phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml to verify, there is one error remaining in \src\Plugin\views\filter\EntityReference.php.

11 | ERROR | [x] Use statements should be sorted alphabetically. The first wrong one is Drupal\Core\Extension\ModuleHandlerInterface.

Nikolay Shapovalov → changed the visibility of the branch 3361499-phpcs-issue-on to hidden.

Thanks everyone for contributions, but I get lost in this issue.
What is the workflow used in this issue?

@Nishant created the Merge Request, but @paraderojether later created patch.
I don't see any explanation why workflow was changed.
https://www.drupal.org/docs/develop/git/using-git-to-contribute-to-drupa... →

I hide branch 3361499-phpcs-issue-on, because no changes were made in the branch.
And as mentioned at #26 I will move Status to Needs work.

I add interdiff file for #25.

Since there is an issue fork, let's use that instead or patches.

I cloned the project and ran the command phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml

Found no phpcs errors

Hi,
I ran this command phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml verf/ and found following warnings.

FILE: /verf/src/Plugin/views/filter/EntityReference.php
------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------
 135 | WARNING | t() calls should be avoided in classes, use \Drupal\Core\StringTranslation\StringTranslationTrait and
     |         | $this->t() instead
------------------------------------------------------------------------------------------------------------------------


FILE: /verf/tests/src/Functional/IntegrationTest.php
---------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 2 WARNINGS AFFECTING 2 LINES
---------------------------------------------------------------------------------------------------------
 171 | WARNING | Unused variable $referencingPublished.
 175 | WARNING | Unused variable $referencingUnpublished.
---------------------------------------------------------------------------------------------------------

Hi,
I checked for phpcs issues for MR!9 using phpcs --standard=Drupal,DrupalPractice --extensions=php,module,inc,install,test,profile,theme,css,info,txt,md,yml verf/
Found no errors and warnings.

For the MR !9, GitLAb CI does not show PHP_CodeSniffer warnings/errors.