fix trigger calls from scheduler

[jasverix]

It is called with call_user_func_array(), and that just does not work with arrays using key indexes. Adding another array around will make sure that the array is sent as first argument to event listener.

It actually crashes in php 8.

[jasverix]

Upgraded my php-resque to my fork now, it works in php8. It may be more things hidden, but this PR is really the only one totally blocking php8 upgrade, so it would be nice if you could merge. I am convinced that this approach was the intended one, it was just forgotten to add another array around, and php7 did not fail. I don't actually know what php7 did there 😅 Maybe just passed the arguments in random order, based on internal array index, or something.

[PATROMO]

@jasverix that is not a fix but a breaking change. You get the errors because PHP 8: supports named arguments. Check this: https://3v4l.org/NfDbn

[jasverix]

Explain how this worked in php7 then. How is key-indexed arrays handled by call_user_func_array?

I am certain that this is a bug, compared to other calls to the trigger method.

[PATROMO]

In PHP7, the index of an associative array is simply ignored. Or is a warning or similar thrown somewhere in php7?
Your change is a breaking change and changes the order of arguments passed. Any code outside would no longer run with your update and would need to be adjusted.
We also run resque and the scheduler in PHP8 and have no error. What exactly kind of error are you getting? Can you post the message here.

[jasverix]

It is as you said, because of named arguments. I am listening to the event in question. If there is no listeners, the trigger will not be called. Just add a listener handler with no arguments.

[mfn]

From my side this is also a breaking change we can't "just do".

Would be great if you could share more details of the error you experienced, like error message and stack trace,including a test which shows, etc.

Also, you shouldn't include all the formatting changes, for now use https://github.com/resque/php-resque/pull/46/files?w=1 to see the actual change.

[jasverix]

Yeah, sorry. PhpStorm trimmed whitespace from all empty lines. I can revert them, and add a test to prove what is failing.

I understand that it is a breaking change, so it needs to be a part of a changelog or something. But the current solution just does not work, not in php8, and it works randomly in php7, so you should consider fix it somehow.

This short code will trigger the error in PHP 8. Can be written anywhere before scheduler work method is ran.

Resque_Event::listen('beforeDelayedEnqueue', function() { echo 'event triggered'; });

I'll revert my whitespace changes, and see if I can write a simple test. But it is a little hard, because I cannot find any unit-tests for scheduler at all.

[jasverix]

Hi,

Whitespaces are fixed. But I cannot see how to unit-test the scheduler without doing some serious refactoring, because it accesses static methods and redis directly, so it is hard to dependency-inject anything there.

I do agree however that this is a breaking change, so it should be a new version with php 8 support maybe?

As I wrote last time, this code breaks in PHP8

Resque_Event::listen('beforeDelayedEnqueue', function() { echo 'event triggered'; });

I can write it like this however:

Resque_Event::listen('beforeDelayedEnqueue', function($queue, $class, $args) { echo 'event triggered'; });

then it will work in PHP 8. So you can consider what you think is better. I'll leave this last comment here though, in case other people have problems using the scheduler in PHP 8 and searches in Google, they may arrive here 😄

[jasverix]

I have rewritten my source code to have the exact same arguments as passed by the array, function($queue, $class, $args), so it will work.

Thank you for your time, and your effort in maintaining this repository. I'll close this PR now.