CommentFileSizeAuthor
#17 interdiff.txt452 bytespratik_kamble
#17 3105977-17.patch775 bytespratik_kamble
#14 interdiff.txt637 bytespratik_kamble
#14 3105977-14.patch1.44 KBpratik_kamble
#8 interdiff-3105977-5-8.txt140 bytesHardik_Patel_12
#8 3105977-8.patch2.22 KBHardik_Patel_12
#5 interdiff-3105977-2-5.txt285 bytesHardik_Patel_12
#5 3105977-5.patch2.27 KBHardik_Patel_12
#2 3105977-2.patch2.22 KBHardik_Patel_12
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Hardik_Patel_12 created an issue. See original summary.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
FileSize
3105977-2.patch2.22 KB

Kindly review a new patch

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
Assigned: Hardik_Patel_12 » Unassigned
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Priority: Normal » Minor
Status: Needs review » Needs work 
  1. +++ b/core/modules/block/block.post_update.php
@@ -86,7 +86,7 @@ function block_post_update_fix_negate_in_conditions() {
+    foreach ($block->getVisibilityConditions() as  $condition) {

    Extra space after "as" 

  2. +++ b/core/modules/block/src/Plugin/migrate/process/BlockPluginId.php
@@ -76,7 +76,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
+          list($type) = explode('-', $delta);

    I realise this is unused but the code is easier to read with it in place.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
FileSize
3105977-5.patch2.27 KB
interdiff-3105977-2-5.txt285 bytes

Kindly reveiw a new patch.

@longwave

I realise this is unused but the code is easier to read with it in place.

, yes you are right but i thought it's good idea
to remove variable if it is not used.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 5: 3105977-5.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
FileSize
3105977-8.patch2.22 KB
interdiff-3105977-5-8.txt140 bytes

Kindly review new patch , bymistakly added space near php tag.

Hardik_Patel_12’s picture

Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented
Status: Needs work » Needs review
pandaski’s picture

pandaski CreditAttribution: pandaski commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/block/block.post_update.php
@@ -86,7 +86,7 @@ function block_post_update_fix_negate_in_conditions() {
-    foreach ($block->getVisibilityConditions() as $condition_id => $condition) {
+    foreach ($block->getVisibilityConditions() as $condition) {

Looks good here since the array key $condition_id not being used

+++ b/core/modules/block/src/Plugin/migrate/process/BlockPluginId.php
@@ -76,7 +76,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
-          list($type, $id) = explode('-', $delta);
+          list($type) = explode('-', $delta);
           if ($type == 'feed') {

Looks good here since $id not being used

+++ b/core/modules/block/tests/src/Functional/BlockTest.php
+++ b/core/modules/block/tests/src/Functional/BlockTest.php
@@ -417,7 +417,7 @@ public function testBlockCacheTags() {

@@ -417,7 +417,7 @@ public function testBlockCacheTags() {
     // Place the "Powered by Drupal" block another time; verify a cache miss.
...
+    $this->drupalPlaceBlock('system_powered_by_block', ['id' => 'powered-2']);

Looks good here since $block2 never being used

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/block/block.post_update.php
@@ -86,7 +86,7 @@ function block_post_update_fix_negate_in_conditions() {
       $configuration = $condition->getConfiguration();

Per other issues specifying the key helps with readability and people often do this intentionally even if the key isn't actually used. If we're going to make this a coding-style rule we should open a proper coding standards issue to do so.

Rangaswini’s picture

Rangaswini CreditAttribution: Rangaswini at QED42 for Drupal India Association commented
Assigned: Unassigned » Rangaswini
pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
Assigned: Rangaswini » pratik_kamble
pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
FileSize
3105977-14.patch1.44 KB
interdiff.txt637 bytes

Rerolled patch to keep the key in foreach.

pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
Status: Needs work » Needs review
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
+++ b/core/modules/block/src/Plugin/migrate/process/BlockPluginId.php
@@ -76,7 +76,7 @@ public function transform($value, MigrateExecutableInterface $migrate_executable
-          list($type, $id) = explode('-', $delta);
+          list($type) = explode('-', $delta);

$id adds context here, I think it is more readable with it in place.

edit: I already said that in #4 :)

pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
FileSize
3105977-17.patch775 bytes
interdiff.txt452 bytes

@longwave updated the Patch to keep $id.

pratik_kamble’s picture

pratik_kamble
Location Pune, India
CreditAttribution: pratik_kamble at QED42 commented
Assigned: pratik_kamble » Unassigned
longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Title: Remove unused variables from block module » Remove unused variable from block module
Status: Needs review » Reviewed & tested by the community

Thanks, the one remaining one is fine.

  • catch committed 228df49 on 9.0.x 
    Issue #3105977 by Hardik_Patel_12, pratik_kamble, longwave, pandaski:...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Fixed

Committed 228df49 and pushed to 9.0.x. Thanks!

  • catch committed 94f1e5d on 8.9.x 
    Issue #3105977 by Hardik_Patel_12, pratik_kamble, longwave, pandaski:...

Status: Fixed » Closed (fixed)

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