preaction
commented
6 years ago For the external API, this 100% must be done how Minion::Backend::Pg (the official backend) does it. So, if it's got bare returns, we can do bare returns. If it has explicit undef, we've got to do explicit undef. The surrounding Minion classes expect the sub to return either a list or a scalar, and we've got to give it what it expects.
The problem with defaulting to list context is that it can also create undesired behavior: Bare return can cause problems when a single, explicit value was expected:
my %vars = (
worker_info => $minion->worker(1)->info,
);With return undef, that does what we expect: Worker 1 doesn't exist. With return, it turns into an odd-sized list (which Perl will warn us about). This was the cause of the Bugzilla security vulnerability a couple years back, and a few years worth of under-informed security-related bashing of Perl by Netanel Rubin.
I recall sri being proactive when that Bugzilla issue hit, so he probably went through and made sure every method had a single expected return context.
For the internal API (sub _update), it doesn't largely matter, but I tend to make sure that subroutine names that seem singular return singular things. _update doesn't sound like it should return anything, but it's only working with one thing, so it makes sense that one thing should be returned (the thing after we've updated it).
So, if Minion::Backend::Pg also does bare return, then we must. Otherwise, we must keep it as-is to keep compatibility with the API.
paultcochrane
Explicitly returning undef can cause a subtle bug, where the return value evaluates as true instead of false as intended. It turns out that using the default return behaviour is actually safer, and hence why this is marked as a problem at the default perlcritic level.
This could be considered a controversial change, nevertheless, I thought I'd submit it just in case you want perlcritic compatability. If you don't want the change, just close unmerged, I won't be offended :-)