Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix deprecated $php_errormsg in PHP 7.2 #7

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ziegenberg
Copy link

@ziegenberg ziegenberg commented Jan 9, 2022

From the PHP 8.0 migration guide (https://www.php.net/manual/de/migration80.incompatible.php):

The track_errors ini directive has been removed.
This means that $php_errormsg is no longer available.
The error_get_last() function may be used instead.

Also the documentation on $php_errormsg (https://www.php.net/manual/en/reserved.variables.phperrormsg.php) says:

This feature has been DEPRECATED as of PHP 7.2.0.
Relying on this feature is highly discouraged.
Use error_get_last() instead.

The functions Horde_String::convertCharset and Horde_String::_pos are broken up to assist in better testability and additional tests for the newly introduced functions are added.

This commit breaks compatibility with PHP 5.6 as error_clear_last() was introduced with PHP 7.

@bytestream
Copy link

bytestream commented Jan 9, 2022

A test could be helpful - if possible - something that at least covers the code if not directly tests the issue raised here 👀

I don't recall this coming up when running the tests on PHP 8.0 or 8.1

@marinaglancy
Copy link

I don't think it's possible. All these changes are made to the if-blocks that check that some php extensions are not available (iconv/mbstring/intl). If you had testing environment without these extensions, many other tests would fail

@ziegenberg
Copy link
Author

I guess, one could mock Horde_Util::extensionExists() like so:

$util = $this->getMockBuilder(Horde_Util::class)
    ->setMethods(array('extensionExists'))
    ->getMock();
$map = [
    ['xml', false],
    ['iconv', true],
    ['mbstring', false]
];
$util->expects($this->any())
    ->method('extensionExists')
    ->will($this->returnValueMap($map));

and then there still needs to be a test developed.

I'm currently challenged with installing the Horde Git Tools. The reason is: composer version 2 dropped pear support... 😐

@bytestream
Copy link

Yeah never mind; it looks like the function would need to be broken up to make it easier to test. Thanks for looking into it.

@yunosh
Copy link
Member

yunosh commented Jan 12, 2022

This isn't anything we could apply until we finally drop PHP 5.6 support. Which is overdue, admittedly.

@ziegenberg
Copy link
Author

People are preparing for PHP 8, starting to leave PHP 7 behind some them soon. Is there a roadmap for dropping support for PHP 5.6? Can you tell a date and time for the transition? Some might start looking for replacement frameworks :(

@yunosh
Copy link
Member

yunosh commented Jan 13, 2022

Yes, we'll drop PHP 5 support with Horde 6.

@ziegenberg
Copy link
Author

Is there an ETA for Horde 6?

From the PHP 8.0 migration guide (https://www.php.net/manual/de/migration80.incompatible.php):
    The track_errors ini directive has been removed.
    This means that php_errormsg is no longer available.
    The error_get_last() function may be used instead.

Also the documentation on $php_errormsg (https://www.php.net/manual/en/reserved.variables.phperrormsg.php) says:
    This feature has been DEPRECATED as of PHP 7.2.0.
    Relying on this feature is highly discouraged.
    Use error_get_last() instead.

The functions Horde_String::convertCharset and Horde_String::_pos are broken up
to assist in better testability and additional tests for the newly introduced
functions are added.

This commit breaks compatibility with PHP 5.6

Signed-off-by: Daniel Ziegenberg <[email protected]>
@ziegenberg ziegenberg force-pushed the replace-deprecated-php_errormsg-with-error_get_last branch from 83e4365 to 39c627e Compare January 16, 2022 18:37
@ziegenberg
Copy link
Author

@bytestream I amended my pull request and broke up the two functions to make it easier to test and also added some basic tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants