Problem/Motivation

When running the tests using Drupal 9 (and 10) a bunch of deprecation warnings are triggered. Things like type hinting, and using assert methods that are deprecated (and removed in Drupal 10). We should get these cleaned up as part of making sure we can create a Drupal 10 compatible release.

Steps to reproduce

Run the test suite using Drupal 9.x.

Issue fork recurly-3326253

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

eojthebrave created an issue. See original summary.

eojthebrave opened merge request !7

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented

On my local two of the tests fail when running with PHP8+ because of an issue in the v2 Recurly PHP client. https://github.com/recurly/recurly-client-php/issues/637, related to calling a deprecated library function. There's a PR to fix it, but it's been open for over a year so who knows ... https://github.com/recurly/recurly-client-php/pull/642 -- I think this an issue we will have to address but it's separate from this one. I think the long-term solution there is to update the module to use v3 of the Recurly PHP client, or to fork the v2 client.

So, I expect the tests to pass fine with PHP7.4 but have a 2 failures due to deprecations with PHP8. Let's see what the test bot says ...

eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented
Status: Active » Needs review

There we go. Sheesh. This now gets the tests currently included with this module passing in Drupal 9 with PHP7.4, 8.0, and 8.1. The issues addressed are:

  • Removing use of deprecated assert methods. Deprecated in Drupal 9. Removed in Drupal 10.
  • In PHP 8.0 the function libxml_disable_entity_loader() is deprecated, the Recurly v2 PHP client which we use needs to be updated to address this. But hasn't been yet. So I've added some logic to the tests to account for this. https://github.com/recurly/recurly-client-php/issues/637
  • In PHP 8.1 return type checking for methods is stricter. You can either add an annotation to the code to skip it, or fix the return type. However, the code that needs updating is in the Recurly library. So I've also added some logic to the tests to account for that.

I think the logic added to our tests for the Recurly client should be treated as "Hey, we know the Recurly client is throwing some deprecation notices, but it still works fine" And as long as you're suppressing deprecation notices on you live site it's fine. The issue is that phpunit doesn't have a way to 'suppress deprecations' so we have to tell it we know about them and we're okay with it.

Getting these tests fixed will allow us to do a couple things:

  • Turn on testing for PHP8+ and Drupal 9
  • Start work on a Drupal 10 release with known good tests
  • Continue the work in #3164892: Improve test coverage and pave the way to make the move to the Recurly v3 client
blakehall’s picture

blakehall commented

This looks good to me, and is also working on my local.

I think having solid (passing) test coverage across PHP versions is a good thing, and think this should be merged.

blakehall’s picture

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

  • eojthebrave committed 47ca7c3c on 4.x 
    Issue #3326253 by eojthebrave, blakehall: Fix deprecation warnings in...
eojthebrave’s picture

eojthebrave
he/him
Primary language English
Location Minneapolis, MN
commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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