Patch attached replaces the usage of deprecated functions and methods with the appropriate replacements.

For more details on ideas and strategies, see https://www.drupal.org/node/3032484

Missing drupalci.yml

CommentFileSizeAuthor
#21 interdiff.txt379 bytesandriansyah
#21 3037323-21.patch3.96 KBandriansyah
#21 3037323-21-ci.patch4.61 KBandriansyah
#16 3037323-16.patch3.99 KBandypost
#16 3037323-16-ci.patch4.63 KBandypost
#16 interdiff.txt770 bytesandypost
#15 3037323-15.patch3.24 KBandypost
#15 3037323-15-ci.patch3.88 KBandypost
#15 interdiff.txt417 bytesandypost
#14 3037323-14-ci.patch3.48 KBandypost
#14 interdiff.txt920 bytesandypost
#13 3037323-13-ci.patch2.73 KBandypost
#12 3037323_12.patch17.4 KBsaidatom
#11 3037323_11.patch16.75 KBsaidatom
#6 3037323-6.patch15.96 KBJeroenT
#6 interdiff-3037323-5-6.txt9.68 KBJeroenT
#5 3037323-5-ci.patch7.02 KBJeroenT
#4 3037323-4-ci.patch6.61 KBandypost
#4 interdiff.txt1.41 KBandypost
#3 3037323-3-ci.patch5.36 KBandypost
#3 3037323-3.patch4.71 KBandypost
remove-usage-deprecated-code.patch5.19 KBJeroenT
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

JeroenT created an issue. See original summary.

Martijn de Wit’s picture

Martijn de Wit
Primary language Dutch
Location 🇳🇱 The Netherlands
CreditAttribution: Martijn de Wit at Ablerz commented
Issue tags: +Drupal 9 compatibility, +midcamp2019
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
3037323-3.patch4.71 KB
3037323-3-ci.patch5.36 KB

Makes sense to add https://www.drupal.org/node/3032484 also rerolled after #3046085: Update deprecated Unicode::* to mb_* for Drupal 9

Meantime getting following

email_registration$ ~/bin/drupal-check.phar -ad -vv -n .
 4/4 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%  1 sec/1 sec 

 ------ ---------------------------------------------------------------------------------------------------------------------------------- 
  Line   email_registration.install                                                                                                        
 ------ ---------------------------------------------------------------------------------------------------------------------------------- 
  11     Function email_registration_requirements not found while trying to analyse it - autoloading is probably not configured properly.  
  30     Function email_registration_update_8100 not found while trying to analyse it - autoloading is probably not configured properly.   
 ------ ---------------------------------------------------------------------------------------------------------------------------------- 

 ------ ------------------------------------------------------------------------------ 
  Line   src/Plugin/Commerce/CheckoutPane/EmailRegistrationLogin.php                   
 ------ ------------------------------------------------------------------------------ 
  27     \Drupal calls should be avoided in classes, use dependency injection instead  
  57     \Drupal calls should be avoided in classes, use dependency injection instead  
  83     Variable $username might not be defined.                                      
  89     Variable $username might not be defined.                                      
  92     Variable $username might not be defined.                                      
  94     Variable $username might not be defined.                                      
  98     Variable $username might not be defined.                                      
  100    Variable $username might not be defined.                                      
 ------ ------------------------------------------------------------------------------ 

                                                                                                                        
 [ERROR] Found 10 errors
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt1.41 KB
3037323-4-ci.patch6.61 KB

Fix cs

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented
FileSize
3037323-5-ci.patch7.02 KB

Patch no longer applied. Created reroll.

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented
FileSize
interdiff-3037323-5-6.txt9.68 KB
3037323-6.patch15.96 KB

Did some extra checks:

vendor/bin/phpcs --standard=vendor/drupal/coder/coder_sniffer/Drupal/ modules/email_registration/

FILE: /private/var/www/drupal8.7/modules/email_registration/README.txt
----------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
----------------------------------------------------------------------
 38 | WARNING | Line exceeds 80 characters; contains 177 characters
----------------------------------------------------------------------

Time: 361ms; Memory: 8MB

vendor/bin/phpstan analyse modules/email_registration/
 4/4 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

                                                                                                                       
 [OK] No errors                                                                                                         

$ drupal-check . -ad -vv -n
 4/4 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100% < 1 sec/< 1 sec

                                                                                                                        
 [OK] No errors
greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Issue summary: View changes

Just spent some time reviewing and this seems like a good idea.

I wonder about doing just this as a first step:

Use the patch that is mentioned in that blogpost (https://www.drupal.org/files/issues/2018-12-17/3020957-2.patch), create an issue in the project, with a title like "Test for Drupal 9 deprecations" or similar. Make sure there is no existing issue yet before doing that. Note that this approach only works if a project has automated tests.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Proudly found elsewhere - d-org ci supports phpstan, see patch in https://www.drupal.org/project/examples/issues/3014492

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Except of config object looks good

+++ b/src/Plugin/Commerce/CheckoutPane/EmailRegistrationLogin.php
@@ -18,14 +27,55 @@ use Drupal\Core\Url;
+    $this->emailRegistrationConfig = $configFactory->get('email_registration.settings');

nor sure that caching of config is good idea

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented

@andypost,

I think I don't understand what you mean.

I'm not against changing, but I think this is a commonly used pattern. Even in core, e.g. Drupal\system\PathBasedBreadcrumbBuilder:

  public function __construct(RequestContext $context, AccessManagerInterface $access_manager, RequestMatcherInterface $router, InboundPathProcessorInterface $path_processor, ConfigFactoryInterface $config_factory, TitleResolverInterface $title_resolver, AccountInterface $current_user, CurrentPathStack $current_path, PathMatcherInterface $path_matcher = NULL) {
    $this->context = $context;
    $this->accessManager = $access_manager;
    $this->router = $router;
    $this->pathProcessor = $path_processor;
    $this->config = $config_factory->get('system.site');
    $this->titleResolver = $title_resolver;
    $this->currentUser = $current_user;
    $this->currentPath = $current_path;
    $this->pathMatcher = $path_matcher ?: \Drupal::service('path.matcher');
  }
saidatom’s picture

saidatom
Location Lisbon
CreditAttribution: saidatom as a volunteer commented
Related issues: +#3093666: Change the way how translation is made
FileSize
3037323_11.patch16.75 KB
saidatom’s picture

saidatom
Location Lisbon
CreditAttribution: saidatom as a volunteer commented
Issue summary: View changes
FileSize
3037323_12.patch17.4 KB
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
3037323-13-ci.patch2.73 KB

Re-roll and removal of (string) as they not needed (if you know why they can be required please comment)

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt920 bytes
3037323-14-ci.patch3.48 KB

Fix one more CS

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt417 bytes
3037323-15-ci.patch3.88 KB
3037323-15.patch3.24 KB

And the last bit

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
FileSize
interdiff.txt770 bytes
3037323-16-ci.patch4.63 KB
3037323-16.patch3.99 KB

And composer changes too

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles at Morris Animal Foundation commented

These changes look good to me.

To really avoid problems for users of 8.7.x, I propose we commit this when 8.7 is no longer supported (or nearly no longer supported).

John Cook’s picture

John Cook CreditAttribution: John Cook at Creode commented
Status: Needs review » Reviewed & tested by the community

I've done another check for patch #16 with upgrade status:

$ drush us-a email_registration    
 [notice] Processing /var/www/drupalvm/drupal/web/modules/contrib/email_registration.
 7/7 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%


================================================================================
Email Registration,  8.x-1.0
Scanned on Fri, 05/22/2020 - 08:10

No known issues found.

As "drupal/core": "^8.7.7 || ^9" is specified in composer.json, I think it's OK to apply now.

Setting to RTBC.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented 
+++ b/email_registration.info.yml
@@ -1,6 +1,6 @@
 dependencies:
-  - drupal:user
+  - drupal:user (>=8.7.7)

the only questionable change, as user module required probably we can remove it

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented
Status: Reviewed & tested by the community » Needs work

Since we added core_version_requirement: ^8.7.7 || ^9 we can changedrupal:user (>=8.7.7) to drupal:user

Setting back to needs work for that change.

andriansyah’s picture

andriansyah CreditAttribution: andriansyah as a volunteer commented
FileSize
3037323-21-ci.patch4.61 KB
3037323-21.patch3.96 KB
interdiff.txt379 bytes

attached the patch file with no version in the drupal:user dependency.

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented
Status: Needs work » Needs review

The last submitted patch, 21: 3037323-21-ci.patch, failed testing. View results

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented
Status: Needs review » Reviewed & tested by the community

The -ci.patch file is failing because of the following deprecation:


Remaining deprecation notices (7)
  7x: Theme functions are deprecated in drupal:8.0.0 and are removed from drupal:10.0.0. Use Twig templates instead of theme_inline_entity_form_entity_table(). See https://www.drupal.org/node/1831138
    1x in EmailRegistrationLoginTest::testLoginWithMailAddress from Drupal\Tests\email_registration\Functional\Plugin\Commerce\CheckoutPane
    1x in EmailRegistrationLoginTest::testLoginWithUsername from Drupal\Tests\email_registration\Functional\Plugin\Commerce\CheckoutPane
    1x in EmailRegistrationLoginTest::testLoginWithoutValues from Drupal\Tests\email_registration\Functional\Plugin\Commerce\CheckoutPane
    1x in EmailRegistrationLoginTest::testFailedLogin from Drupal\Tests\email_registration\Functional\Plugin\Commerce\CheckoutPane
    1x in EmailRegistrationLoginTest::testFailedLoginWithUsername from Drupal\Tests\email_registration\Functional\Plugin\Commerce\CheckoutPane
    1x in EmailRegistrationLoginTest::testFailedLoginWithMailAddressWhenUsernameIsAllowed from Drupal\Tests\email_registration\Functional\Plugin\Commerce\CheckoutPane
    1x in EmailRegistrationLoginTest::testFailedLoginWithMailAddressWithBlockedUser from Drupal\Tests\email_registration\Functional\Plugin\Commerce\CheckoutPane

which is a deprecation in inline_entity_form that should be fixed before Drupal 10: #3121071: Replace theme_inline_entity_form_entity_table() with twig template.

JeroenT’s picture

JeroenT
Primary language Dutch
Location 🇧🇪
CreditAttribution: JeroenT at iO commented

As mentioned in #17 this should be committed once 8.7 is no longer supported. Since Drupal 8.7 is currently no longer supported we can probably commit this now.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

++ to get in and maybe few other issues and create new release

  • andypost committed 9978585 on 8.x-1.x authored by andriansyah 
    Issue #3037323 by andypost, JeroenT, andriansyah, saidatom: Remove usage...
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Reviewed & tested by the community » Fixed

I went ahead and commited #21 also filed follow-up #3148078: Add custom drupalci.yml

Gonna create new release

Status: Fixed » Closed (fixed)

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