Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pifagor created an issue. See original summary.

pifagor’s picture

Assigned: Unassigned » pifagor
pifagor’s picture

Status: Active » Needs review
FileSize
3.1 KB
pifagor’s picture

Assigned: pifagor » Unassigned
voleger’s picture

Status: Needs review » Reviewed & tested by the community

Nice, simple cleanup. +1 for rtbc

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

This means that Drupal.Classes.UnusedUseStatement is broken - we need to fix this in coder and then update core\s usage of coder and fix it there.

idebr’s picture

Title: Unused import » [PP-1] Unused import
Issue summary: View changes
Status: Needs work » Postponed
Related issues: +#3052377: Drupal.Classes.UnusedUseStatement sniff is incomplete

I filed #3052377: Drupal.Classes.UnusedUseStatement sniff is incomplete in the Coder module to fix the Drupal.Classes.UnusedUseStatement sniff

klausi’s picture

+++ b/core/modules/migrate_drupal_ui/migrate_drupal_ui.module
@@ -6,7 +6,6 @@
-use Drupal\Core\Url;

I don't understand this removal. Url is used in this file so we need the use statement?

pifagor’s picture

Because this file uses "\Drupal::url" and in the "use Drupal\Core\Url;" is no need.

klausi’s picture

I don't think so - the file uses Url::fromRoute(), so we need the use statement? See https://git.drupalcode.org/project/drupal/blob/8.8.x/core/modules/migrat...

pifagor’s picture

Status: Postponed » Needs review
alexpott’s picture

We need to get coder in Drupal core updated to use the new rule - which means we need a release of coder.

klausi’s picture

Title: [PP-1] Unused import » Fix unused imports and update Coder to 8.3.4
Status: Needs review » Needs work

Coder 8.3.4 released, so you can upgrade here I guess?

alexpott’s picture

Well there's also #3049433: Coding standards not running on d.o. - upgrade to coder 8.3.4 - these efforts need merging because upgrading is going to uncover this.

Gábor Hojtsy’s picture

klausi’s picture

Status: Needs review » Needs work

Looks good, just some minor things:

  1. +++ b/composer.lock
    @@ -5357,7 +5357,7 @@
    -        "php": "^5.5.9|>=7.0.8"
    +        "php": ">=7.0.8"
    

    Unrelated change? The PHP requirements of any package should not change with this patch?

  2. +++ b/core/tests/Drupal/KernelTests/Core/Render/Element/TableTest.php
    @@ -236,7 +236,7 @@ public function testThemeTableHeaderRenderArray() {
    -      'two',
    +       'two',
    

    wrong change, the opening bracket a couple lines above needs to be fixed instead.

  3. +++ b/core/tests/Drupal/KernelTests/Core/Render/Element/TableTest.php
    @@ -276,7 +276,7 @@ public function testThemeTableRowRenderArray() {
    -        '2-two',
    +         '2-two',
    

    same here.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
20.63 KB
1.34 KB

Addressed comments #18 in updated patch & added an interdiff as well.

klausi’s picture

Status: Needs review » Needs work

Thanks a lot, this is almost ready, just one minor thing:

+++ b/composer.lock
@@ -3759,12 +3759,12 @@
                 "type": "git",
-                "url": "https://github.com/bovigo/vfsStream.git",
...
                 "reference": "d5fec95f541d4d71c4823bb5e30cf9b9e5b96145"
             },
             "dist": {
                 "type": "zip",
-                "url": "https://api.github.com/repos/bovigo/vfsStream/zipball/d5fec95f541d4d71c4823bb5e30cf9b9e5b96145",
+                "url": "https://api.github.com/repos/mikey179/vfsStream/zipball/d5fec95f541d4d71c4823bb5e30cf9b9e5b96145",

sorry, just also checked these changes and they are also unrelated and should be removed. https://github.com/bovigo/vfsStream is the new home of that package and https://github.com/mikey179/vfsStream redirects there, so we should not change it in our lock file because it is already correct.

idebr’s picture

#18.1 Drupal Core changed its PHP requirement in composer.json in #3053363: Remove support for PHP 5 in Drupal 8.8, but the composer.lock file was not updated with this change. The change is therefore correct, and will always show for any `composer update X` call. We can change this in a separate issue if you like?

klausi’s picture

I see, totally missed the PHP 7 upgrade in core (yay!). Please do it in a separate issue so that we keep the changes here small and in scope for this change.

yogeshmpawar’s picture

Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

Assigned: yogeshmpawar » Unassigned
Status: Needs work » Needs review
FileSize
19.89 KB
880 bytes

Comments addressed of @klausi from #21 & added an interdiff as well.

idebr’s picture

#23 Filed #3057575: Removal of PHP5 support is not yet reflected composer.lock to update the PHP requirement in composer.lock

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Perfect, all good to go!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7e08466 and pushed to 8.8.x. Thanks!

  • alexpott committed 7e08466 on 8.8.x
    Issue #3041983 by yogeshmpawar, idebr, pifagor, klausi: Fix unused...
jonathan1055’s picture

Just a question - composer.lock file now loads coder 8.3.4, but the composer.json file still has

        "drupal/coder": "^8.3.2",

Is it OK to leave it like this? I know that ^8.3.2 will be satisfied by 8.3.4 but does it look odd to have the discrepancy?

Status: Fixed » Closed (fixed)

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