PHP compatibility testing reports the following PHP 7 issues:
$ ./vendor/bin/phpcs --standard=./vendor/phpcompatibility/php-compatibility/PHPCompatibility/ruleset.xml --runtime-set testVersion 7.3 web/modules/contrib/htmlmail/
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 1 ERROR AND 1 WARNING AFFECTING 2 LINES
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------
376 | WARNING | INI directive 'safe_mode' is deprecated since PHP 5.3 and removed since PHP 5.4
408 | ERROR | Since PHP 7.0, functions inspecting arguments, like debug_backtrace(), no longer report the original value as passed to a parameter, but will instead
| | provide the current value. The parameter "$message" was changed on line 325.
-----------------------------------------------------------------------------------------------------------------------------------------------------------------------
These issues were addressed in the 7.x version of the module, but have not yet been addressed in 8.x:
- #3138522: WARNING | Since PHP 7.0, functions inspecting arguments, like debug_backtrace()
- #3107966: PHP7 compatibility: INI directive 'safe_mode' is deprecated since PHP 5.3 and removed since PHP 5.4
Apply the same fixes from the 7.x version.
| Comment | File | Size | Author |
|---|---|---|---|
| #9 | 3167046-9.patch | 1.62 KB | bobbygryzynger |
| #8 | 3167046-8-php-7.patch | 1.62 KB | tr |
| #5 | 3167046-2-5.interdiff.txt | 842 bytes | bobbygryzynger |
| #5 | 3167046-5.patch | 1.54 KB | bobbygryzynger |
| #2 | 3167046-2.patch | 1.4 KB | bobbygryzynger |
Comments
Comment #2
bobbygryzyngerAdding patch with ported 7.x version fixes.
Comment #3
bobbygryzyngerComment #4
tr commentedYeah, that's why the Drupal policy is to make fixes in the current version first, and THEN backport fixes if desired. Fixing it in D7 first means that the D8 version is going to be WORSE than the D7 version, especially since D7 patches often don't get ported to D8 like you have demonstrated here. I'm mentioning that because next time someone objects to me moving issues to D8 I'm going to point them to this problem - this is exactly why we should stick with Drupal policy.
With the patch in #2, PHP discontinued safe_mode as of PHP 5.4. So while we need to deal with lower versions of PHP in D7, we don't have to worry about that anymore in D8 - D8 now requires PHP 7. Because of this, any code dealing with safe_mode should just be removed, we no longer have to check to see if safe_mode is set.
Comment #5
bobbygryzyngerMakes sense @TR. I updated the patch to remove the safe_mode check.
Comment #6
bobbygryzyngerComment #7
tr commentedThanks. I reviewed the old D7 issues and I re-opened #3138522: WARNING | Since PHP 7.0, functions inspecting arguments, like debug_backtrace() because I do not think the fix in that issue was a good way to fix the problem. I would like to re-visit that issue and find a better solution. That means putting this current issue on hold for a while until that is figured out, as I want to make sure what we do is the same in D7 and D8.
Comment #8
tr commentedOK, more things:
The conditional that checks on this was clearly meant for much older versions of PHP - even Drupal 7 requires at least PHP 5.3. Thus, we no longer need this conditional and the block of code inside it.
debug_backtrace(0), I don't think we need to move the debug_backtrace() call to the beginning of the method. debug_backtrace() is expensive, and should only be used when we need it. That is, when debugging is turned on. Right now, the patch in #5 calls debug_backtrace() for every single email.@bobbygryzynger: Can you please run your PHP compatibility tests and see if this patch resolves the issue?
Comment #9
bobbygryzynger@TR I wasn't able to apply #8 to the alpha2 version, so I rerolled it against that version. After that, all compatibility testing is clean.
Comment #10
tr commentedThanks! Yes, #8 only applies to the current 8.x-3.x-dev, because that is what the testbot needs to test the patch. There have been a lot of changes since the alpha2 release, so when I get all the D9 issues fixed I will be making a new release.
Comment #12
tr commentedCommitted.