resquebundle / resque

ResqueBundle for Symfony 4+
MIT License
50 stars 29 forks source link

Update attachRetryStrategy #41

Closed mbourqui closed 5 years ago

mbourqui commented 5 years ago

Either this line is not supposed to be there, or the above if-loop is pointless.

PhilETaylor commented 5 years ago

As https://github.com/resquebundle/resque/blob/master/README.markdown states right near the end, the retry strategy should be an array, even if the array is a blank array (which should overrule the global retry strategy)

Therefore I think the count() lines should be there, and the other line should be removed as proposed.

mbourqui commented 5 years ago

But then precisely, shouldn't the test just be is_array? Otherwise, if it is an empty array, the count() will return 0 which is evaluated as false, and the global config will be set.

PhilETaylor commented 5 years ago

im looking into this today

mbourqui commented 5 years ago

Well, after looking more closely at it, it works as explained in the docs. The only difference is that if the array is empty, the $job->args['resque.retry_strategy'] will not be set. I do not know though if this is mandatory.

PhilETaylor commented 5 years ago

sorry was just typing that out :) too... thats the bit im testing now, I dont think its needed.

PhilETaylor commented 5 years ago

its used here https://github.com/resquebundle/resque/blob/762727ad1a2e06465e73ef8af282514f794795f3/bin/resque#L108

and a quick test proves no error/warning will be generated if not set

<?php
$args = [];
var_dump(empty($args['resque.retry_strategy']));
mbourqui commented 5 years ago

Great, seems we're all good then!

mbourqui commented 5 years ago

Thanks for double-checking and sorry for the doubt.