YaLinqo icon indicating copy to clipboard operation
YaLinqo copied to clipboard

PHP 7.2 Deprecations: create_function

Open fandrieu opened this issue 7 years ago • 34 comments

As as follow-up to #24: the create_function has been deprecated in php 7.2.

Would it be possible to update Utils::createLambdaFromString to avoid using that function ?

Thanks

fandrieu avatar Feb 15 '18 09:02 fandrieu

The only alternative to create_function is eval which doesn't allow caching of compiled code, so causes considerably worse performance if the same string lambda is called more than once with different arguments. This deprecation doesn't make any sense to me, it just removes a feature for the sake of removing a feature. The reasoning given in the manual is pure bullshit.

My advice would be:

  1. If you want to preserve the performance provided by caching of evaluated string lambdas and find the performance hit of string lambdas acceptable, you should disable deprecation warnings in php.ini.

  2. If you want maximum performance available with YaLinqo, or you can't modify PHP's deprecation settings, you should follow the advice given by PHP manual and use anonymous functions.

In either case, if you know a way to convince stupid bullheaded PHP devs to finally implement damn arrow functions which has been in RFC for ages and has been requested since forever, please do so. If these retarded fucks finally implement arrow functions, I can finally drop support for string lambdas and forget this abomination ever existed.

If I understand the deprecation process correctly, leaving create_function in the code doesn't cause any real issues right now, but will when the deprecated function is eventually fully removed in the next major PHP release. When that happens, I'll switch to using eval, or drop support for string lambdas completely if arrow functions are implemented. I'll keep the issue open to keep track of the status of create_function.

Note that some string lambdas like '$v' or '$k' are cached internally by YaLinqo, so you can keep using them in place of more verbose static methods of the Functions class and even more verbose anonymous functions without causing deprecation warning to be emitted by PHP.

Athari avatar Feb 15 '18 11:02 Athari

The benchmark shows that is slightly faster to create a function with create_function than use regular closures. This could be a reason why YaLinqo is faster when string functions are used. So there could be a performance penalty if create_function will be removed.

sanmai avatar Mar 17 '18 15:03 sanmai

The create_function invocation has been silenced (@) in master via 7591c4d and #30, but there is still no tag to accommodate the change, so all projects using YaLinqo fail on PHP 7.2 with Function create_function() is deprecated. @Athari Can we get a new tag?

Bilge avatar Dec 09 '18 17:12 Bilge

@Bilge You have a point. I'll add a minor 2.x version I think, as master is currently unfinished 3.0.

Hm, PHP 7.3 is available. Thank gods, they didn't remove deprecated functions. That would happen only in 8.0, right? I hope that by that time, these idiots will finally add arrow functions instead of implementing random garbage like instanceof on literals and numeric keys in objects. Ugh.

Hm, looks like I need to remove support for real type. Will be removed in 7.4.

Athari avatar Dec 12 '18 10:12 Athari

Yes, they would never remove any deprecated functions in a minor release. But that's besides the point, the deprecation warnings themselves are the problem in this case.

Bilge avatar Dec 12 '18 18:12 Bilge

If you need to upgrade more than one create_function to anonymous function (official recommended upgrade), you can use a CLI tool for that.

It's tested on 30 various (and really weird :)) cases.

-$callback = create_function('$a', 'return "<cas:proxy>$a</cas:proxy>";');    
+$callback = function ($a) {
+    return "<cas:proxy>{$a}</cas:proxy>";
+};

Includes concat (.), string quotes and inclined function calls:

-$func = create_function('$atts, $content = null','return "<div class=\"' . $class_list . '\">" . do_shortcode($content) . "</div>";' );
+$func = function ($atts, $content = null) use ($class_list) {
+    return "<div class=\"{$class_list}\">" . do_shortcode($content) . "</div>";
+};

Do you want to automate the hard work?

1. Instal Rector

composer require rector/rector --dev

2. Create config

# rector.yml
services:
    Rector\Php\Rector\FuncCall\CreateFunctionToAnonymousFunctionRector: ~

3. Upgrade your Code

vendor/bin/rector process src --config rector.yml --dry-run
vendor/bin/rector process src --config rector.yml

Enjoy!

TomasVotruba avatar Dec 16 '18 01:12 TomasVotruba

@Athari It's been a month since I raised this. Any update on the minor tag?

Bilge avatar Jan 06 '19 21:01 Bilge

@Bilge Back-ported changes from master which affect supported PHP versions, added tag v2.4.2. I may have broken something in the process (messed up rebase, had to reset branches), so please test.

Athari avatar Jan 10 '19 20:01 Athari

Looking at a0e782a I can say there's one thing missing surely. If you don't test on PHP 5.6, do not declare support for it. #43 should fix that problem, if there is a problem (I'm not entirely sure what went where, GitHub isn't helping).

(CI's failing on this commit. I think I've fixed these non-numeric values before, should be easy to cherry-pick. Though master build is all green, so guess it's all right.)

sanmai avatar Jan 11 '19 00:01 sanmai

@TomasVotruba IIRC create_function was faster on benchmarks than a modern anonymous function.

sanmai avatar Jan 11 '19 00:01 sanmai

How is that relevant in case of removed function?

TomasVotruba avatar Jan 11 '19 00:01 TomasVotruba

Sure, when it's removed, it's removed. When it's not, @create_function is not as bad. (I'd run another benchmark to see how worse it is with an at mark, though.)

sanmai avatar Jan 11 '19 00:01 sanmai

Piling up deprecations didn't proove as good practice. Better solve 5 deprecations per minor version, than 20 at once.

TomasVotruba avatar Jan 11 '19 00:01 TomasVotruba

Any news on this? I am still getting a lot of warnings on v2.4.2.

andre719mv avatar Aug 31 '19 14:08 andre719mv

Shall we hard-fork this?

sanmai avatar Sep 02 '19 03:09 sanmai

@andre719mv @sanmai What is your problem exactly? The code works. The error is muted. If you don't use string lambdas, the deprecated function in never executed.

Where do you get "a lot of warnings" from? Especially in plural form. From analysis tools?

PHP 7.4 still hasn't been released. I don't see any benefits of releasing "YaLinqo 3.0" with the only feature being "removed feature" to satisfy a deprecation in a minor release of PHP 7.2, just to release YaLinqo 4.0 a month later with proper argument type hints which would make sense in PHP 7.4 supporting arrow functions.

If you prefer to inconvenience yourself with forks instead of configuring your analysis tools, feel free to do so. In the meantime, I can offer adding a branch where create_function is replaced with throwing an exception, so you can switch to that instead of a named package version, if that's what you're looking for. However, if you give me one good reason to bump a major version number just for deprecation, I'll reconsider my plans of making YaLinqo 3.0 a version switching to stricter argument typing, dropping string lambdas in favor of arrow functions and making a couple of long overdue breaking changes (#41, #11 etc.).

Athari avatar Sep 02 '19 08:09 Athari

Hi! Yes, I have some analysis tool on my site to monitor all errors and warnings. Warnings from this lib has filled a lot of space and it`s hard to see other problems. image

Thanks for explanation. Now I see that there is no good way to fix it before PHP 7.4. No problem. I`ll add exception to analysis tool.

andre719mv avatar Sep 02 '19 13:09 andre719mv

Version numbers are free.

On Mon, 2 Sep 2019, 14:12 andre719mv, [email protected] wrote:

Hi! Yes, I have some analysis tool on my site to monitor all errors and warnings. Warnings from this lib has filled a lot of space and it`s hard to see other problems. [image: image] https://user-images.githubusercontent.com/22563994/64102938-a5ecb080-cd79-11e9-84ae-5213a904e24d.png

Thanks for explanation. Now I see that there is no good way to fix it before PHP 7.4. No problem. I`ll add exception to analysis tool.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/Athari/YaLinqo/issues/25?email_source=notifications&email_token=AADS4YXHH5QSNUCLKG5HQD3QHUGMRA5CNFSM4EQ2HST2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5VZF5A#issuecomment-527143668, or mute the thread https://github.com/notifications/unsubscribe-auth/AADS4YVUHIYHGZ2E5WO4IHLQHUGMRANCNFSM4EQ2HSTQ .

Bilge avatar Sep 02 '19 13:09 Bilge

And warnings are not. Here's a benchmark I cooked from a thin air:

$start = microtime(true);

for ($i = 0; $i < 1000; $i++) {
    $fn = @create_function('$a,$b', 'return log($a * $b);');
}

var_dump(microtime(true) - $start);

$start = microtime(true);

for ($i = 0; $i < 1000; $i++) {
    $fn = function ($a, $b) {return log($a * $b);};
}

var_dump(microtime(true) - $start);

Code with create_function is about 100 times slower because of the muted warning.

Output for 7.3.9
float(0.015002012252808)
float(0.00026297569274902)

See for yourself: https://3v4l.org/13afF

If create_function was faster before, not anymore now.

sanmai avatar Sep 04 '19 09:09 sanmai

@sanmai Your synthetic test has nothing in common with how the library works. The function create_function allows caching of the compiled result, so you need to call it once per each unique string lambda. On the other hand, the not yet deprecated function eval forces you to compile the function for every single call, no matter whether it's unique or not. On top of this, it also does not support arguments, so you're forced to serialize values into code.

Let's run a test which reflects how the library actually works:

<?php

$start = microtime(true);
$fn = @create_function('$a,$b', 'return log($a * $b);');
$v = 0;
for ($i = 0; $i < 1000; $i++)
    $v += $fn($i, $i);
echo("create_function: " . (microtime(true) - $start) . "\n");

$start = microtime(true);
$v = 0;
for ($i = 0; $i < 1000; $i++)
    $v += eval("return log($i * $i);");
echo("eval: " . (microtime(true) - $start) . "\n");

$start = microtime(true);
$fn = fn($a, $b) => log($a * $b);
$v = 0;
for ($i = 0; $i < 1000; $i++)
    $v += $fn($i, $i);
echo("arrow function: " . (microtime(true) - $start) . "\n");

$start = microtime(true);
$fn = function ($a, $b) { return log($a * $b); };
$v = 0;
for ($i = 0; $i < 1000; $i++)
    $v += $fn($i, $i);
echo("anonymous function: " . (microtime(true) - $start) . "\n");

Output:

create_function: 0.00020408630371094
eval: 0.0031659603118896
arrow function: 0.00015783309936523
anonymous function: 0.00012993812561035

As you can see, the "eval" version is horrendously slow. "Fixing" the deprecation warning by following the suggestion from the developers of PHP and replacing create_function with eval would make the library 15x times slower. It would also be a major breaking change as very few values can be easily serialized for eval. Obviously, this is not an option, as it would make the library unusable for anyone who relies on string lambdas.

The "create_function" version is still slower than anonymous and arrow functions (1.5x), of course, but it's a known compromise between performance and conciseness. YaLinqo as a whole is a compromise between performance and readability as you do lose performance compared to direct built-in function calls.

It's typical for developers of PHP to break things before fixing and then break them again. In this case, they decided to deprecate a function before providing a valid alternative. PHP 7.4 has breaking changes too, which aren't suited for a minor version release, and it breaks more than a dozen of top packages. That's just how PHP is developed.

Athari avatar Sep 04 '19 10:09 Athari

Kind reminder =). PHP 7.4 has been released on November 28, 2019

andre719mv avatar Feb 08 '20 19:02 andre719mv

Working on it.

Athari avatar Feb 20 '20 00:02 Athari

With PHP 8.0 release, this is broken! The code no longer works.

vspl-girish avatar Nov 30 '20 05:11 vspl-girish

+1 for fixing this to gain PHP8 compatibility

seiz avatar Feb 18 '21 10:02 seiz

This library is abandoned.

Bilge avatar Feb 18 '21 10:02 Bilge

Although the library seems to be dead, I successfully managed to convert the few calls in my code that triggered the usage of create_function.

This was before:

$r->OptionValues = 
    E::from($model->Options)->select('$v->Value')->toArray();

And this is how I migrated it to:

$r->OptionValues = 
    E::from($model->Options)->select(function ($v) {return $v->Value;})->toArray();

I.e. I replaced the string with my code, that was converted by the YaLinqo to a function by simply providing the anonymous function by myself.

As of now, this seems to work just perfectly. Hopefully it stays this way.

UweKeim avatar Mar 07 '21 16:03 UweKeim

Thanks for the workaround. Successfully implemented it and got our code to work!

vspl-girish avatar Mar 08 '21 10:03 vspl-girish

I think that that is the only solution going forward. By the way, you can shorten the lamdba function by using fn ($v) => $v->Value.

jorrit avatar Mar 08 '21 10:03 jorrit

@jorrit Is this "short syntax" compatible with older PHP versions?

According to https://www.php.net/manual/en/functions.arrow.php this only works with PHP 7.4 and newer.

Unfortunately I have to support older versions, too.

UweKeim avatar Mar 08 '21 10:03 UweKeim

You're right about that. My suggestion is just for codebases that only need to support 7.4 and up.

jorrit avatar Mar 08 '21 10:03 jorrit