Upgrading to PHP 8.0 using Drupal core 7.91 and TFA Basic 7.x-1.1+1-dev I get the following errors:

FILE: tfa_basic/includes/googleauthenticator/GoogleAuthenticator.php
FOUND 3 ERRORS AFFECTING 1 LINE
 37 | ERROR | Function mcrypt_create_iv() is deprecated since PHP 7.1 and removed since PHP 7.2; Use random_bytes() or OpenSSL instead
 37 | ERROR | Extension 'mcrypt' is deprecated since PHP 7.1 and removed since PHP 7.2; Use openssl (preferred) or pecl/mcrypt once available instead
 37 | ERROR | The constant "MCRYPT_DEV_URANDOM" is deprecated since PHP 7.1 and removed since PHP 7.2

Because we are using Authenticated AES (Real AES) we could remove the code referring to the deprecated functions and the errors got fixed.

CommentFileSizeAuthor
#2 tfa_basic-3310662-2.patch909 bytesalina.basarabeanu

Comments

Alina Basarabeanu created an issue. See original summary.

alina.basarabeanu’s picture

alina.basarabeanu commented
StatusFileSize
new tfa_basic-3310662-2.patch909 bytes
poker10’s picture

poker10 commented
Status: Active » Needs work

Thanks for working on this.

This is not a real problem, because the function call is executed only if the function_exists() check passes, so in case the function does not exists (PHP 8+), then the function is not used at all. It can cause PHPCS issues, but we could add // phpcs:ignore to ignore these. It is a better solution than removing it altogether, as the modules is supposed to support also PHP 5.x and PHP 7.x versions.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

I think it would also be appropriate to drop php 5 support at this point. I believe core is doing it soon. Would be better to match core than litter comments in the code, in my opinion.

poker10’s picture

poker10 commented
Status: Needs work » Needs review

@greggles Yes, it is true that D7 core is dropping support for PHP lower than 5.6 (and possibly for PHP 5.6 also), but dropping that support does not mean that the D7 core would stop working on these ancient versions (immediately) - because we are not going to add backwards incompatible changes intentionally, if not needed (such as short array syntax and so on).

I prefer this approach also in contrib modules - if it is possible to keep the module usable on older versions, then OK. Sadly we do not have any telemetry regarding PHP/OS/SQL, so we can only guess how many sites are up-to-date and how many on ancient shared hostings. These decisions would be a lot easier if we had such statistics. I agree, that this is a more conservative approach, but yeah, I am "trained" on D7 core :)

Currently, the module offers three options:

1. random_bytes - PHP 7+
2. mcrypt_create_iv - PHP 4, PHP 5, PHP 7 < 7.2 + PECL extension
3. openssl_random_pseudo_bytes - PHP 5.3+

To run this module on PHP 5.6 or lower we need mcrypt_create_iv or openssl_random_pseudo_bytes. The mcrypt extension was a most universal option used for ages, so there could be sites relying on this (not saying it is good - it is not). But technically it is still possible to use mcrypt on PHP 8.3 via PECL extension.

But if we guess that most of the sites using this module have openssl enabled (which probably should have, as they are adding additional security feature, so why would they use ancient library on the other side?), then yes, we can probably remove the usage of mcrypt_create_iv, as it was proposed in the patch #2.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

OK if that appeals to you that's fine by me :)

poker10’s picture

poker10 commented
Status: Needs review » Fixed

Committed, thanks all!

Status: Fixed » Closed (fixed)

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