新增auth支持#328
Conversation
|
One of the changes here seems helpful, namely the use of PID tracking to ensure better "thread safety" across forks. That said, all the rest are breaking changes. Some bugfixes are reverted, and many features are removed. I suspect the changes in this PR were made to a much older version of the code than the most recent version in Additionally, your second commit changes the namespace of the package, which cannot be accepted. To address this, please close this PR, create a new branch even with the |
danhunsaker
left a comment
danhunsaker
left a comment
There was a problem hiding this comment.
Changing the namespace is not something that can be merged.
danhunsaker
left a comment
danhunsaker
left a comment
There was a problem hiding this comment.
One other note that isn't specific to any given line - please make sure you use consistent spacing and indentation in PRs. I see at least one case where you fixed the indentation, but several more where you've added something with poor spacing.
| <?php | ||
| require_once dirname(__FILE__) . '/Resque/Event.php'; | ||
| require_once dirname(__FILE__) . '/Resque/Exception.php'; | ||
|
|
There was a problem hiding this comment.
These lines are from before Composer was introduced...
| * and implement "thread" safety to avoid race conditions. | ||
| */ | ||
| protected static $pid = null; | ||
|
|
There was a problem hiding this comment.
This seems useful, though.
| * a callable that receives the configured database ID | ||
| * and returns a Resque_Redis instance, or | ||
| * @param mixed $server Host/port combination separated by a colon, or | ||
| * a nested array of servers with host/port pairs. |
There was a problem hiding this comment.
Dropping a lot of valid server definitions, here...
| return $pid; | ||
| self::$redis->select(self::$redisDatabase); | ||
| return self::$redis; | ||
| } |
There was a problem hiding this comment.
The Resque::fork() method is pretty essential to the way the library operates now. It shouldn't be removed.
| } | ||
| return true; | ||
| self::redis()->rpush('queue:' . $queue, json_encode($item)); | ||
| } |
There was a problem hiding this comment.
This removes a lot of error handling...
| { | ||
| $counter = self::size($queue); | ||
| $result = self::redis()->del('queue:' . $queue); | ||
| return ($result == 1) ? $counter : 0; |
There was a problem hiding this comment.
Again, we still need to be able to remove jobs from the queue.
| */ | ||
| public static function generateJobId() | ||
| { | ||
| return md5(uniqid('', true)); |
There was a problem hiding this comment.
Not strictly vital, but consistency is nice, so putting this in a method rather than duplicating it everywhere is helpful.
|
|
||
| return $pid; | ||
| self::$redis->select(self::$redisDatabase); | ||
| return self::$redis; |
There was a problem hiding this comment.
All of this logic is unnecessary with Credis, and is in fact redundant. Assuming it dates from before the switch to Composer, as well.
| $pid = getmypid(); | ||
| if (self::$pid !== $pid) { | ||
| self::$redis = null; | ||
| self::$pid = $pid; |
There was a problem hiding this comment.
This bit still looks helpful, though.
| { | ||
| self::$redisServer = $server; | ||
| self::$redisDatabase = $database; | ||
| self::$auth = $auth; |
There was a problem hiding this comment.
I can see why this might be tempting to have. Feels a bit superfluous, though.
|
I'm sure this is obvious in the code, but the PR title translates to Added auth support |
2 participants
No description provided.