Skip to content
This repository was archived by the owner on Jun 9, 2021. It is now read-only.

add option to turn on/off version check #9

Closed
wants to merge 2 commits into from
Closed

add option to turn on/off version check #9

wants to merge 2 commits into from

Conversation

khsing
Copy link

@khsing khsing commented Nov 29, 2016

for some cloud memcached service doesn't return correct version, but function is normal. Add a option able to skip version check. default version check is enable.

@@ -37,8 +37,10 @@ public function connect(
);
}
}
// some cloud memcached service don't return correct version, so skip version check.
$checkVersion = in_array("check_version",$options) ? $options["check_version"] : FALSE;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • No space found after comma in function call
  • TRUE, FALSE and NULL must be lowercase; expected false but found FALSE

@@ -97,15 +99,15 @@ protected function setCredentials($memcached, $credentials)
* @param \Memcached $memcached
* @return \Memcached
*/
protected function validateConnection($memcached)
protected function validateConnection($memcached, $checkVersion=TRUE)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Incorrect spacing between argument $checkVersion and equals sign; expected 1 but found 0
  • Incorrect spacing between default value and equals sign for argument $checkVersion; expected 1 but found 0
  • TRUE, FALSE and NULL must be lowercase; expected true but found TRUE

@@ -21,7 +21,8 @@ class MemcachedConnector
*/
public function connect(
array $servers, $connectionId = null,
array $options = [], array $credentials = []
array $options = [], array $credentials = [],

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Multi-line function declarations must define one parameter per line

@@ -97,15 +98,15 @@ protected function setCredentials($memcached, $credentials)
* @param \Memcached $memcached
* @return \Memcached
*/
protected function validateConnection($memcached)
protected function validateConnection($memcached, $checkVersion=true)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Incorrect spacing between argument $checkVersion and equals sign; expected 1 but found 0
  • Incorrect spacing between default value and equals sign for argument $checkVersion; expected 1 but found 0

@tomcastleman
Copy link
Owner

@khsing this package supports memcached servers using ext-memcached. I would suggest any memcached servers returning invalid version information do not conform to the protocol and therefore are unsupported. If you would like to follow this up I would ask that you provide further information on the cloud provider in question.

Thanks

@khsing
Copy link
Author

khsing commented Apr 14, 2017

@tomcastleman I am using Aliyun OCS service as memcached alternative service, which build up with alibaba/tair.

Aliyun OCS does not return memcached version, but totally compatible with memcached protocol.

@tomcastleman
Copy link
Owner

Possibly related laravel/framework issue.

@khsing
Copy link
Author

khsing commented Apr 18, 2017

@tomcastleman yes, same issues. ;-)

@tomcastleman
Copy link
Owner

Fixed in #10

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants