Skip to content

Remove some deprecated functions #4927

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Remove some deprecated functions #4927

wants to merge 7 commits into from

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Nov 17, 2019 •
edited
Loading

I noticed while I was working on the stub for the implode() function that some of the deprecations of https://wiki.php.net/rfc/deprecations_php_7_4 haven't been implemented yet for PHP 8.

I hope it's not a problem that I wanted to help with the implementation.

TysonAndre
TysonAndre reviewed Nov 17, 2019
View reviewed changes
@TysonAndre
Copy link
Contributor

TysonAndre commented Nov 17, 2019 •
edited
Loading

Also, I think reflection can be changed for implode() after this/along with this.

Look at the recent PRs to ext/standard/basic_functions.stub.php for this - e.g. https://github.com/php/php-src/pull/4772/files#r331638679

// contents of  ext/standard/basic_functions.stub.php
/* array.c */

function array_push(array &$stack, ...$args): int {}

Second, there are some test failures to check out for implode() - I only checked travis

========DIFF========
001+ Fatal error: Uncaught TypeError: The first argument must be string in /home/travis/build/php/php-src/ext/spl/tests/bug75717.php:8
002+ Stack trace:
001- some nested items
002- some nested items
003+ #0 /home/travis/build/php/php-src/ext/spl/tests/bug75717.php(8): join(NULL, Array)
004+ #1 /home/travis/build/php/php-src/ext/spl/tests/bug75717.php(17): flatten(Array)
005+ #2 {main}
006+   thrown in /home/travis/build/php/php-src/ext/spl/tests/bug75717.php on line 8
========DONE========

TysonAndre
TysonAndre reviewed Nov 17, 2019
View reviewed changes
ext/standard/string.c
php_error_docref(NULL, E_DEPRECATED,
"Passing glue string after array is deprecated. Swap the parameters");
} else if (Z_TYPE_P(arg2) == IS_ARRAY) {
if (Z_TYPE_P(arg1) == IS_STRING) {
glue = zval_get_tmp_string(arg1, &tmp_glue);
Copy link
Contributor

@TysonAndre TysonAndre Nov 17, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also get rid of tmp_glue and zend_tmp_string_release, because Z_PARAM_STR gives you the zend_string

@cmb69
Copy link
Member

cmb69 commented Nov 17, 2019

Thanks for working on this @kocsismate!

cmb69
cmb69 reviewed Nov 17, 2019
View reviewed changes
ext/standard/string.c
Converts logical Hebrew text to visual text with newline conversion */
PHP_FUNCTION(hebrevc)
{
php_hebrev(INTERNAL_FUNCTION_PARAM_PASSTHRU, 1);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to me that we now can inline php_hebrev() into PHP_FUNCTION(hebrev).

Copy link
Member Author

@kocsismate kocsismate Nov 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Just committed it.

nikic
nikic reviewed Nov 18, 2019
View reviewed changes
ext/opcache/Optimizer/zend_func_info.c
@@ -178,9 +178,6 @@ static const func_info_t func_infos[] = {
F1("str_split", MAY_BE_FALSE | MAY_BE_ARRAY | MAY_BE_ARRAY_KEY_LONG | MAY_BE_ARRAY_OF_STRING),
F1("strpbrk", MAY_BE_FALSE | MAY_BE_STRING),
F0("substr_compare", MAY_BE_FALSE | MAY_BE_LONG),
#ifdef HAVE_STRFMON
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should also drop the strfmon check in configure.

nikic
nikic reviewed Dec 5, 2019
View reviewed changes
ext/standard/string.c
php_error_docref(NULL, E_DEPRECATED,
"Passing glue string after array is deprecated. Swap the parameters");
} else if (Z_TYPE_P(arg2) == IS_ARRAY) {
if (Z_TYPE_P(arg1) == IS_STRING) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, this changes the behavior: Previously it would cast anything (that isn't an array) to a string, now it strictly requires a string. Ideally it would follow zpp rules...

@nikic
Copy link
Member

nikic commented Dec 5, 2019

Apart from implode, this looks fine. Please commit the rest with appropriate notes in UPGRADING.

@nikic
Copy link
Member

nikic commented Dec 5, 2019

I've opened #4970 as preparation for handling implode properly.

@kocsismate
Copy link
Member Author

Thanks Nikita! I saw the PR is merged now, so I'll make use of it.

@kocsismate
Copy link
Member Author

kocsismate commented Dec 5, 2019 •
edited
Loading

I am closing this PR (and open a separate one for implode()) via the following commits:
b63c625 64468d1 144b41c 6339260 29ef077 b95da3c 73730ee

@kocsismate kocsismate closed this Dec 5, 2019
@kocsismate kocsismate deleted the deprecations-74 branch December 5, 2019 13:28
@kocsismate kocsismate mentioned this pull request Dec 12, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic nikic left review comments

@TysonAndre TysonAndre TysonAndre left review comments

@cmb69 cmb69 cmb69 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kocsismate @TysonAndre @cmb69 @nikic