Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zaporylie created an issue. See original summary.

zaporylie’s picture

Status: Active » Needs review
FileSize
28.86 KB
borisson_’s picture

Status: Needs review » Needs work

Tested this with ag

ag "@var (.*)\."

core/tests/Drupal/Tests/Core/Asset/CssOptimizerUnitTest.php
23:   * @var \Drupal\Core\Asset\CssOptimizer object.

core/tests/Drupal/Tests/Core/Asset/CssCollectionGrouperUnitTest.php
18:   * @var \Drupal\Core\Asset\CssCollectionGrouper object.

core/tests/Drupal/Tests/Core/Asset/JsOptimizerUnitTest.php
18:   * @var \Drupal\Core\Asset\JsOptimizer object.

core/tests/Drupal/Tests/Core/Asset/CssCollectionRendererUnitTest.php
18:   * @var \Drupal\Core\Asset\CssRenderer object.

core/tests/Drupal/Tests/BrowserTestBase.php
142:   * @var string.

core/modules/quickedit/tests/src/Kernel/MetadataGeneratorTest.php
33:   * @var \Drupal\quickedit\MetadataGeneratorInterface.php

core/modules/views/src/Plugin/views/field/MachineName.php
21:   * @var array Stores the available options.

core/modules/editor/tests/src/Kernel/QuickEditIntegrationTest.php
41:   * @var \Drupal\quickedit\MetadataGeneratorInterface.php

Not sure if we should fix those @var declarations that have a php file as well in this issue, but the other ones should still be fixed.

This patch is also fixing semicolons, those are all ok.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

idebr’s picture

Status: Needs work » Needs review
FileSize
4.83 KB
32.46 KB

#3 Fixed the occurrences mentioned in the comment, but I was unable to install ag on my Windows machine to check for new violations. Can you take a look?

idebr’s picture

Status: Needs review » Needs work

Found a few more:

FILE: ...jects\d8\core\modules\ckeditor\tests\src\Kernel\CKEditorTest.php
----------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
----------------------------------------------------------------------
 27 | ERROR | [x] Expected "\Drupal\ckeditor\Plugin\Editor\CKEditor"
    |       |     but found
    |       |     "\Drupal\ckeditor\Plugin\Editor\CKEditor;" for @var
    |       |     tag in member variable comment
 34 | ERROR | [x] Expected "\Drupal\editor\Plugin\EditorManager" but
    |       |     found "\Drupal\editor\Plugin\EditorManager;" for
    |       |     @var tag in member variable comment

FILE: ...:\Users\Ide\Projects\d8\core\modules\comment\src\CommentForm.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 44 | ERROR | [x] Expected
    |       |     "\Drupal\Core\Entity\EntityFieldManagerInterface"
    |       |     but found
    |       |     "\Drupal\Core\Entity\EntityFieldManagerInterface;"
    |       |     for @var tag in member variable comment


FILE: ...jects\d8\core\modules\migrate\tests\src\Unit\MigrateTestCase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 32 | ERROR | [x] Expected
    |       |     "\Drupal\migrate\Plugin\MigrationInterfaceSTATUS_"
    |       |     but found
    |       |     "\Drupal\migrate\Plugin\MigrationInterface::STATUS_*"
    |       |     for @var tag in member variable comment

FILE: ...odules\migrate_drupal_ui\src\Batch\MigrateUpgradeImportBatch.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 63 | ERROR | [x] Expected
    |       |     "\Drupal\migrate\Plugin\MigrationInterface[]" but
    |       |     found
    |       |     "\Drupal\migrate\Plugin\MigrationInterface[];" for
    |       |     @var tag in member variable comment

FILE: ...Projects\d8\core\tests\Drupal\KernelTests\ConfigFormTestBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 17 | ERROR | [x] Expected "\Drupal\Core\Form\FormInterface" but
    |       |     found "\Drupal\Core\Form\FormInterface." for @var
    |       |     tag in member variable comment


FILE: ...s\d8\core\tests\Drupal\Tests\Core\Asset\CssOptimizerUnitTest.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 23 | ERROR | [x] Expected "\Drupal\Core\Asset\CssOptimizerobject"
    |       |     but found "\Drupal\Core\Asset\CssOptimizer object"
    |       |     for @var tag in member variable comment
idebr’s picture

Status: Needs work » Needs review
FileSize
3.12 KB
35.05 KB

Fixed the violations mentioned in #6

borisson_’s picture

Status: Needs review » Reviewed & tested by the community

I tried to run composer phpcs after applying this patch and couldn't find any remaining issues.

catch’s picture

Status: Reviewed & tested by the community » Needs work

The patch looks fine except shouldn't it also enable the rule?

zaporylie’s picture

zaporylie’s picture

Status: Needs work » Reviewed & tested by the community
larowlan’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/migrate/tests/src/Unit/MigrateTestCase.php
@@ -29,7 +29,7 @@
-   * @var \Drupal\migrate\Plugin\MigrationInterface::STATUS_*
+   * @var \Drupal\migrate\Plugin\MigrationInterface::STATUS_IDLE

Shouldn't this just be 'int'?

zaporylie’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
35 KB
522 bytes

From MigrationInterface.php:

/**
   * Set the current migration status.
   *
   * @param int $status
   *   One of the STATUS_* constants.
   */
  public function setStatus($status);

Therefore, I think #12 is a good idea. Moving back to RTBC.

larowlan’s picture

Adding review credit for @borisson_ who did manual ag work to verify

larowlan’s picture

Status: Reviewed & tested by the community » Fixed

Committed as 0d6d22a and pushed to 8.6.x.

  • larowlan committed 0d6d22a on 8.6.x
    Issue #2926120 by idebr, zaporylie, borisson_: @var tag must not end...

Status: Fixed » Closed (fixed)

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