Problem/Motivation

Drupal 9 is built in Drupal 8 by deprecating APIs. Module Upgrader should not produce suggested transformations that are not Drupal 9 compatible.

Proposed resolution

Test with password_reset_tabs and see what the results look like.

Remaining tasks

Review. Fix. Commit.

User interface changes

None.

API changes

TBD.

Data model changes

TBD.

Release notes snippet

N/A

CommentFileSizeAuthor
#17 interdiff-3057394-17.txt761 bytesjoshi.rohit100
#17 3057394-17.patch12.32 KBjoshi.rohit100
#12 interdiff-3057394-12.txt902 bytesjoshi.rohit100
#12 3057394-12.patch12.39 KBjoshi.rohit100
#9 interdiff-3057394-9.txt6.03 KBjoshi.rohit100
#9 3057394-9.patch11.51 KBjoshi.rohit100
#3 3057394-3.patch5.48 KBjoshi.rohit100
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy created an issue. See original summary.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented
Assigned: Unassigned » joshi.rohit100

I just identified the following transformation not D9 compatible -

  • EntityGetInfo
  • EntityLoad
  • L
  • LoadMultiple
  • NodeLoad
  • UserLoad
  • UserSave
  • CommentLoad
  • EntityCreate

Will raise a patch for them.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented
Assigned: joshi.rohit100 » Unassigned
Status: Active » Needs review
FileSize
3057394-3.patch5.48 KB

Patch uploaded.

Sample code that I tried -

function test_function() {
  $entity_info = entity_get_info('node');

  $node = node_load(5);

  $node = entity_load($node);

  l('text', '/node/add');

  $node = node_load_multiple([1, 2]);

  $node = node_load(5);

  $user = user_load(4);

  user_save($user);

  $comment = comment_load(5);

  $entity = entity_create('node', []);
}

"UserSave" transformation is working fine, so there is no change required for it.

amitgoyal’s picture

amitgoyal CreditAttribution: amitgoyal commented
Status: Needs review » Needs work

Thanks @joshi.rohit100 for providing the patch.

Before applying the patch, I was getting below 5 errors,

$ drupal-check -d modules/password_reset_tabs/
 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ------------------------------------------------------------
  Line   password_reset_tabs.module
 ------ ------------------------------------------------------------
  103    Call to deprecated method entityManager() of class Drupal.
  191    Call to deprecated method entityManager() of class Drupal.
  196    Call to deprecated function drupal_set_message().
  231    Call to deprecated method entityManager() of class Drupal.
 ------ ------------------------------------------------------------

 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   src/Routing/RouteSubscriber.php
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------
         Class Drupal\password_reset_tabs\Routing\RouteSubscriber was not found while trying to analyse it - autoloading is probably not configured properly.
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------


 [ERROR] Found 5 errors

After applying the patch, I am getting below 2 errors,

$ drupal-check -d modules/password_reset_tabs/
 2/2 [▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓▓] 100%

 ------ ---------------------------------------------------
  Line   password_reset_tabs.module
 ------ ---------------------------------------------------
  196    Call to deprecated function drupal_set_message().
 ------ ---------------------------------------------------

 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------
  Line   src/Routing/RouteSubscriber.php
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------
         Class Drupal\password_reset_tabs\Routing\RouteSubscriber was not found while trying to analyse it - autoloading is probably not configured properly.
 ------ ------------------------------------------------------------------------------------------------------------------------------------------------------


 [ERROR] Found 2 errors

Looks like patch has fixed some of the issues but still some work needs to be done.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

I don't think all the Drupal 9 compatibility issues will be resolvable here with the module upgrader. I think opening multiple issues so they can be committed one by one would be better. There will definitely be db_* API issues for example. See #3033163: drupal_set_message is deprecated, replace it with MessengerInterface::addMessage() for an existing issue about a Drupal 9 compatibility issue.

Also, before more commits can be made to the module, its tests need to be made to pass unfortunately :/ See https://www.drupal.org/node/2196489/qa I opened #3072671: DMU 8.x-1.x branch test fails 20 times just now.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs work » Needs review

Now that branch tests passed, sent this for a retest.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Needs work

Ok some tests needs to be fixed for the new replacements, see the fails :)

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented
Assigned: Unassigned » joshi.rohit100

Assigning myself to work on test failure fixes

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented
Status: Needs work » Needs review
FileSize
3057394-9.patch11.51 KB
interdiff-3057394-9.txt6.03 KB

Fixing test failures.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented
Assigned: joshi.rohit100 » Unassigned

Status: Needs review » Needs work

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

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented
Status: Needs work » Needs review
FileSize
3057394-12.patch12.39 KB
interdiff-3057394-12.txt902 bytes

Fixing remaining failure.

amitgoyal’s picture

amitgoyal CreditAttribution: amitgoyal commented
Status: Needs review » Reviewed & tested by the community

#12 looks good to me. Thanks Rohit!

So far there is only one following deprecation for which we have separate issue - #3033163: drupal_set_message is deprecated, replace it with MessengerInterface::addMessage(),

➜  d9 git:(8.8.x) ✗ ./vendor/bin/drupal-check -d modules/password_reset_tabs


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

 ------ ----------------------------------------------------------------------
  Line   password_reset_tabs.module
 ------ ----------------------------------------------------------------------
  196    Call to deprecated function drupal_set_message():
         in Drupal 8.5.0 and will be removed before Drupal 9.0.0.
         Use \Drupal\Core\Messenger\MessengerInterface::addMessage() instead.
 ------ ----------------------------------------------------------------------


 [ERROR] Found 1 error
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Title: Make sure transformations don't use deprecated APIs, so resulting suggestions are Drupal 9 compatible » Attempt transformations of Drupal 7 version of password_reset_tabs and fix Drupal 9 incompatibilities in the result
Issue summary: View changes

Retitled and edited the issue summary since this issue was taken over for some specific fixes instead of ensuring the whole of transformations are Drupal 9 compatible. Not sure @joshi.rohit100's sample was comprehensive of what DMU supports (he did not indicate it but also did not indicate it was not). That said, the verification from @amitgoyal was based on password_reset_tabs and not a general test scenario.

I recreated the general issue at #3080468: [META] Make sure transformations don't use deprecated APIs, so resulting suggestions are Drupal 9 compatible with the original issue summary and will make this a child.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Parent issue: » #3080468: [META] Make sure transformations don't use deprecated APIs, so resulting suggestions are Drupal 9 compatible
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Reviewed & tested by the community » Needs work 
+++ b/src/Plugin/DMU/Converter/Functions/L.php
@@ -29,7 +29,9 @@ class L extends URL {
-        return ClassMethodCallNode::create('\Drupal', 'l')
+        return ClassMethodCallNode::create('\Drupal', 'service')
+          ->appendArgument('link_generator')
+          ->appendMethodCall('generate')

Why are you not using Link::fromTextAndUrl() as indicated by the deprecation on Drupal:l() itself?

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Link.php/...

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 at Acquia commented
Status: Needs work » Needs review
FileSize
3057394-17.patch12.32 KB
interdiff-3057394-17.txt761 bytes

Changed link_generator service to `Link::fromTextAndUrl()`

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Status: Needs review » Fixed

Superb, thanks! Otherwise looked good, so landed! :)

  • Gábor Hojtsy committed f2ba985 on 8.x-1.x 
    Issue #3057394 by joshi.rohit100, Gábor Hojtsy, amitgoyal: Attempt...

Status: Fixed » Closed (fixed)

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