Skip to content

Conversation

@SidRoberts
Copy link
Contributor

Hello!

In raising this pull request, I confirm the following:

  • I have read and understood the Contributing Guidelines
  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • [-] I have updated the relevant CHANGELOG
  • [-] I have created a PR for the documentation about this change

Two important things of note:

I've tried to alter the code to work around these problems but I'd like someone with a fresh set of eyes to double check as I've been looking at this too long. 😜

Copy link
Contributor

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

In general looks good to me

sergeyklay
sergeyklay previously approved these changes Jul 30, 2019
@sergeyklay
Copy link
Contributor

It's a little bit confusing with the bugs in the getAllKeys() method

@sergeyklay
Copy link
Contributor

@SidRoberts Could you please change memcached version check to reflect this
See: php-memcached-dev/php-memcached#315

Make sure you use php-memcached extension >= 3.0.1 and memcached server >= 1.4.24

@SidRoberts
Copy link
Contributor Author

I agree that this is very confusing. Currently Travis is using Memcached server 1.5.6 and Memcached extension 3.1.3. In theory, it should work but it doesn't.

I don't like the idea of a test doing version checking. I think the version checks should be in the getKeys() method and we should maybe implement code that actually gets all the keys instead of dealing with this mess.

How about keep the version check at >=1.2.24 for now, merge it and then deal with it later?

@SidRoberts
Copy link
Contributor Author

Or, as a last ditch effort, what about bc0712e?

Copy link
Contributor

@CameronHall CameronHall left a comment

Choose a reason for hiding this comment

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

Looking good 👍

@sergeyklay sergeyklay merged commit 0bc942a into phalcon:4.0.x Jul 31, 2019
@sergeyklay
Copy link
Contributor

Thank you

@SidRoberts SidRoberts deleted the mysql-script branch July 31, 2019 11:11
@niden niden added the 4.0 label Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants