Athari / YaLinqo

Yet Another LINQ to Objects for PHP [Simplified BSD]
https://athari.github.io/YaLinqo
BSD 2-Clause "Simplified" License
439 stars 39 forks source link

PHP 7.2 Deprecations: create_function #25

Open fandrieu opened 6 years ago

fandrieu commented 6 years ago

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

Athari commented 6 years ago

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.

sanmai commented 6 years ago

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.

Bilge commented 5 years ago

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?

Athari commented 5 years ago

@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.

Bilge commented 5 years ago

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.

TomasVotruba commented 5 years ago

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!

Bilge commented 5 years ago

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

Athari commented 5 years ago

@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.

sanmai commented 5 years ago

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 commented 5 years ago

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

TomasVotruba commented 5 years ago

How is that relevant in case of removed function?

sanmai commented 5 years ago

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.)

TomasVotruba commented 5 years ago

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

andre719mv commented 5 years ago

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

sanmai commented 5 years ago

Shall we hard-fork this?

Athari commented 5 years ago

@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.).

andre719mv commented 5 years ago

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.

Bilge commented 5 years ago

Version numbers are free.

On Mon, 2 Sep 2019, 14:12 andre719mv, notifications@github.com 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 .

sanmai commented 5 years ago

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.

Athari commented 5 years ago

@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.

andre719mv commented 4 years ago

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

Athari commented 4 years ago

Working on it.

vspl-girish commented 3 years ago

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

seiz commented 3 years ago

+1 for fixing this to gain PHP8 compatibility

Bilge commented 3 years ago

This library is abandoned.

UweKeim commented 3 years ago

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.

vspl-girish commented 3 years ago

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

jorrit commented 3 years ago

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.

UweKeim commented 3 years ago

@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.

jorrit commented 3 years ago

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

flashover commented 1 year ago

Can confirm, using an anonymous function within Yalinqo from($array)->sum(fn(x) => $x->getSomeValue()); works but will there be an official update in this repository for PHP 8? I don't have a need for caching / performance.

Athari commented 1 year ago

Latest news

  1. I updated v2.0 branch with commit tagged v2.5.0. It replaces create_function call with eval call.
  2. My assumtions above about performance issues can be ignored as I assumed some sort of eval($code) while eval("return function($args) { $code };") does what I want exactly how I want.
  3. I haven't used PHP for ages and have no idea what I'm doing. I pushed the code and added the tag. Unit tests seem to work. Composer should notice the update, right? Let me know whether any of this is in any way functional.
  4. The "3.0 massive performance upgrade" is abandoned forever. I failed to implement any remotely useful optimizations in any remotely meaningful logic paths. Maybe it's impossible, maybe I lack the required IQ. Either way, it won't happen.

Future plans

The master branch does contain some useful changes besides failed optimization (most of it isn't even in the repo), including a few extra methods and API fixes (with breaking changes). At the moment I'm considering releasing everything as 3.0. Maybe with extra callable type hints and whataver else newest PHP contains, thus dropping "string lambdas" functionality.

Considering nobody seems to be interested in new functionality, including me, the difference between 2.0 and 3.0 will be 5% cleaner API, 3 extra methods, dropped string lambdas and PHP 8+ requirement.

Let me know what you think. Assuming anyone is still following this issue.

jorrit commented 1 year ago

I think steady maintenance is exactly what this library needs. Just keeping up with the changes in PHP and merging the occasional contribution seems like a good plan. Thanks for your efforts!

Athari commented 1 year ago

Apparently Packagist requires a hook now. It failed to install one automatically, so I added it manually. Seems to work now, the 2.5 version of the package is up.