made signals configurable by event callback

mjphaynes / php-resque

php-resque is a Redis-backed PHP library for creating background jobs, placing them on multiple queues, and processing them later.

MIT License

221 stars 50 forks source link

made signals configurable by event callback #85

Closed scones closed 5 years ago

scones commented 6 years ago

prerequisite for #77

cleaner patch (whatever happened in the other branch, is not happening here)

xelan commented 6 years ago

Thank you very much for this interesting PR, @scones!

Just three small things we should consider before merging:

Should we add tests for this new feature? It would probably make sense to add at least a small test case for each new feature to avoid future breaks.
Does the property signalHandlerMapping need to be protected? As it is a new one, there can't be any subclasses around which need direct access to it. This is something we should maybe address for all properties/methods. Probably it would be wise to reduce the visibility of protected properties/methods to private unless they are extension points for subclasses. However, this can only be done with a new major release.
- At the moment (PHP-Resque 2.x), we have PHP 5.3 as minimum requirement. Either you change the array syntax to the long one or the PR needs to wait until 3.0.

Best regards Andreas

scones commented 6 years ago

To your points:

1) Yes we should, yet i was unfortunately unable to do so, as my php version was 7.0 and i was unable to get the test suite running. I will see, that i get a better container up to add the missing tests

2) i followed the existing pattern. i don't care for the access level either way, just aimed at consistancy

3) i will change it to the compatible method

xelan commented 6 years ago

Thank you 👍

xelan commented 6 years ago

How's your status concerning this PR, @scones?

When it's ready we could include this feature in 2.2.0 already.

scones commented 6 years ago

Hello xelan,

Sorry i have been busy lately with other tasks, so could not fix the issues. I am writing these patches in the company context, thus i am not always able to invest time on this.

best regards, scones

xelan commented 6 years ago

Hi @scones! What do you think, should we get this ready for merge into the 2.x branch before working on the larger 3.0 changes? How's your status with the PR?

xelan commented 6 years ago

Hi @scones! If you change the array syntax and code standard, I could also merge this feature into 2.2, as I do not see a breaking change or another disadvantage by doing so :smile:

xelan commented 5 years ago

This could also be merged in the next 2.x release :smile: @scones Could you please just address the small array syntax and CS issues?

scones commented 5 years ago

should be in compliance now

xelan commented 5 years ago

Thank you, @scones :+1: