Prevent introduction of PHP 8.2 deprecations

Created on 16 March 2023, over 1 year ago
Updated 12 May 2023, about 1 year ago

Problem/Motivation

The module uses some features which are deprecated in PHP 8.2.

<!--break-->

Following is a list of issues detected (no guarantee for completeness):

Steps to reproduce

Install the module on a Drupal (10) site running on PHP 8.2, configure it and send a test email.

Proposed resolution

Fix the deprecations. There might be different solutions depending on the minimum supported PHP version. If PHP 7.4 must be supported, PHP Annotations cannot be used for example as they have been introduced in PHP 8.0. But considering that PHP 7.4 will be EOL by end of 2023, we might require PHP >= 8.0 already.

Remaining tasks

Add this to 🌱 Roadmap Active .

User interface changes

API changes

Data model changes

📌 Task
Status

Active

Version

1.2

Component

Code

Created by

🇦🇹Austria mvonfrie

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @mvonfrie
  • 🇦🇹Austria mvonfrie

    Ok, I just found out that the error why I created ( 🐛 Dynamic property error on PHP 8.2 Fixed ) has been fixed in 1.2.0. But I suggest to keep this to run static code analysis for other deprecations.

  • 🇬🇧United Kingdom AdamPS

    The dynamic properties are an accident - they should have a declaration. However I develop on PHP8.1 so I don't see a warning.

    In the page you linked to it says

    It is possible to programmatically emit a deprecation in older versions in the interest of consistency across multiple PHP versions.

    I'd be very happy with that if you know how to do it.

  • Status changed to Needs review about 1 year ago
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    5 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.0.7 + Environment: PHP 8.2 & MySQL 8
    last update about 1 year ago
    8 fail
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    5 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.0.7 + Environment: PHP 8.2 & MySQL 8
    last update about 1 year ago
    4 pass, 2 fail
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    PHPLint Failed
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.0.7 + Environment: PHP 8.2 & MySQL 8
    last update about 1 year ago
    PHPLint Failed
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    5 pass
  • Open in Jenkins → Open on Drupal.org →
    Core: 10.0.7 + Environment: PHP 8.2 & MySQL 8
    last update about 1 year ago
    5 pass
  • Status changed to Active about 1 year ago
  • 🇦🇹Austria mvonfrie

    I didn't look on this issue for a while, but unfortunatley don't exactly know what it meant by

    It is possible to programmatically emit a deprecation in older versions in the interest of consistency across multiple PHP versions.

    PHPStan though is able to detect these issues, as I just got another one

    Deprecated function: Creation of dynamic property Drupal\symfony_mailer\Processor\EmailBuilderManager::$proxy is deprecated in Drupal\symfony_mailer\Processor\EmailBuilderManager->getProxyMapping() (line 204 of modules/contrib/symfony_mailer/src/Processor/EmailBuilderManager.php).

    which by PHPStan is reported as

     ------ ---------------------------------------------------------------------------------------------- 
      Line   src/Processor/EmailBuilderManager.php                                                         
     ------ ---------------------------------------------------------------------------------------------- 
      204    Access to an undefined property Drupal\symfony_mailer\Processor\EmailBuilderManager::$proxy.  
             💡 Learn more: https://phpstan.org/blog/solving-phpstan-access-to-undefined-property          
     ------ ---------------------------------------------------------------------------------------------- 

    Find the full PHPStan report of the symfony_mailer 1.2.1 attached for reference (as it is too long to post it directly in the comment).

  • 🇬🇧United Kingdom AdamPS

    Thanks. Do you know if we could include phpstan in the automatic tests for this module? As far as I can see, mglaman/phpstan-drupal is required by D10 core, but not D9.

  • 🇬🇧United Kingdom AdamPS

    Looking at phpstan.symfony_mailer.txt I found 4 actual minor bugs, and another maybe D10 bug (MailerPolicyListBuilder). So that's good to know.

    The rest are in various categories

    1. Missing definitions from external code
    2. In theory the warning is true, but in practice it never is
    3. "\Drupal calls should be avoided in classes" which is a reasonable principle but also can cause BC problems and lots of extra code
    4. Limitation/bug of the analysis tool (MailManagerReplacement)

    Possibly in some cases the code could be altered/improved to make the warning go away. So the tool might be useful, but it will require some work.

  • 🇦🇹Austria mvonfrie

    Which warning exactly do you mean with #2? And #3 btw doesn't get triggered by static methods.

  • 🇬🇧United Kingdom AdamPS

    Which warning exactly do you mean with #2

    59 Variable $messages might not be defined.
    38 Variable $addresses might not be defined.
    Etc. Static analysis notices that the loop might never be executed, however I believe the arrays cannot be empty. However I guess it's easy enough to add the code that will fix the warning.

    #3 btw doesn't get triggered by static methods.

    I am aware of that. IMO dependency injection adds a lot of tedious "boiler plate" code, even 20-30 lines just to make use of a single function from a service. Also if you want to call a new service from an existing class, it's not fully BC, as it changes the constructor.

  • 🇦🇹Austria mvonfrie

    If the #3 warning should cause problems (it would block CI if you include phpstan in the automatic tests) this could be a workaround:

    class MyService {
    
      protected static function getMessenger() {
        return \Drupal::messenger();
      }
    }
    

    I made the method protected to help with testing, this way it can be overridden to provide a mocked service instance easily.

    59 Variable $messages might not be defined.

    $messages is assigned inside a nested if statement inside the foreach loop, so the fix is very easy. Just add $messages = []; directly before the foreach loop.

  • 🇬🇧United Kingdom AdamPS

    if you include phpstan in the automatic tests

    Is this possible to do?

    I made the method protected to help with testing, this way it can be overridden to provide a mocked service instance easily.

    Neat.

Production build 0.69.0 2024