Comments

ankithashetty created an issue. See original summary.

ankithashetty’s picture

Assigned: Unassigned » ankithashetty
ankithashetty’s picture

Title: Add core_version_requirement for Drupal 9 compatibility » Drupal 9 Deprecated Code Report
Issue summary: View changes
StatusFileSize
new50.65 KB
ankithashetty’s picture

Assigned: ankithashetty » Unassigned
Status: Active » Needs review
StatusFileSize
new6.42 KB
new1.13 KB

Only local images are allowed.
Replaced deprecated code to make it Drupal 9 ready in the following patch. Kindly review.

Thank you!

jaykandari’s picture

Assigned: Unassigned » jaykandari

On it.

jaykandari’s picture

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

Hi @ankithashetty , Thanks for the patch.

Please find my findings below.

Patch apply fails both ways (via git & patch commands):

simple_social_icons git/8.x-3.x  
❯ git apply 3139275-4.patch 
error: patch failed: simple_social_icons.info.yml:7
error: simple_social_icons.info.yml: patch does not apply

simple_social_icons git/8.x-3.x  
❯ patch -p1 < 3139275-4.patch 
patching file simple_social_icons.info.yml
Hunk #1 FAILED at 7.
1 out of 1 hunk FAILED -- saving rejects to file simple_social_icons.info.yml.rej
patching file src/Plugin/Block/SimpleSocialIconsBlock.php

Upgrade Status reports OK
Only one issue core_version_requirement issue was reported and that is already being covered in the patch #4. Its just that patch apply needs to fix.

Also, there are some minor issues reported by Rector & drupal-check
drupal_set_message() issues are already being fixed..Once the patch applying is successful. I think remaining are just minor issues.

> drupal-check -ad ./modules/custom/simple_social_icons

 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ------------------------------------------------------------------------------ 
  Line   src/Plugin/Block/SimpleSocialIconsBlock.php                                   
 ------ ------------------------------------------------------------------------------ 
  783    Variable $order_icon might not be defined.                                    
  791    Call to deprecated function drupal_set_message():                             
         in drupal:8.5.0 and is removed from drupal:9.0.0.                             
         Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead.          
  1056   \Drupal calls should be avoided in classes, use dependency injection instead  
  1206   Variable $icon_order might not be defined.                                    
  1208   Variable $icon_order might not be defined.                                    
  1255   Variable $icon_order might not be defined.                                    
  1257   Variable $icon_order might not be defined.                                    
 ------ ------------------------------------------------------------------------------ 

                                                                                                                        
 [ERROR] Found 7 errors                                                                                                 
                                                                                                                        

----------

Running Rector... 
> vendor/bin/rector process ./modules/custom/simple_social_icons --dry-run
Rector 0.8.x-dev@80f429b
Config file: rector.yml

 6/6 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

1 file with changes
===================

1) modules/custom/simple_social_icons/src/Plugin/Block/SimpleSocialIconsBlock.php

    ---------- begin diff ----------
--- Original
+++ New
@@ -788,7 +788,7 @@
       }
       else {
         $form_state->setErrorByName($key, $this->t('<b>NOTE: Each icon should have distinct order</b>'));
-        drupal_set_message($this->t('Order of icon is same for @previous_val and @key error',
+        $this->messenger()->addStatus($this->t('Order of icon is same for @previous_val and @key error',
           [
             '@previous_val' => $previous_val,
             '@key' => $key,
    ----------- end diff -----------


Applied rules:

 * DrupalRector\Rector\Deprecation\DrupalSetMessageRector


                                                                                                                        
 [OK] Rector is done! 1 file would have changed (dry-run).                                                              
                                                                                                                        

----------

DONE

ankithashetty’s picture

Status: Needs work » Needs review
StatusFileSize
new2.79 KB

Hello @JayKandari, thank you for reviewing the patch. I have updated a new patch as discussed. Please review the same.

jaykandari’s picture

Status: Needs review » Reviewed & tested by the community

Thanks @ankithashetty for the patch.

#7 applies cleanly now.

No issues Reported by Upgrade status module after applying patch.

Ran rector, No issues reported here also.

❯ vendor/bin/rector process modules/custom/simple_social_icons --dry-run
Rector 0.8.x-dev@80f429b
Config file: rector.yml

 6/6 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                        
 [OK] Rector is done!                                                                                                   
                                                                                                                        

Thus marking it as RTBC.
Thanks !!

caspervoogt’s picture

StatusFileSize
new427 bytes

Patch #7 left the core: 8.x line intact, but that should not remain there. It should be replaced by core_version_requirement: ^8 || ^9 (it can't contain both lines). Working on a patch (ignore the one attached to this comment)

caspervoogt’s picture

caspervoogt’s picture

StatusFileSize
new428 bytes

This one (patch file. #10) applies cleanly for me. It removes the core line and adds a core_version_requirement line instead, since the info file cannot contain both.

Upgrade Status was not complaining about this module before this patch, but when I tried actually upgrading to 9 I kept getting "drupal/simple_social_icons 3.x-dev requires drupal/core ~8.0" errors. I was hoping my patch would solve that, but I think it will only be truly solved once this is committed, because I think when I run "composer require drupal/core:9.2.6" it actually checks the Simple Social Icons Git repo and the repo tells Composer this module requires Drupal 8, not 9.

The Drupal 9 deadline is just a month away. Can we get this committed please?

shelane’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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