Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix stubs to make them consistent for PHP 8 and add a checkKey() method along with a bug fix #479

Closed
wants to merge 4 commits into from

Conversation

rlerdorf
Copy link
Contributor

@rlerdorf rlerdorf commented Feb 25, 2021

These are marked as nullable in the arginfo files in the tree. Without this change regenerating the arginfo with the PHP 8 gen_stubs will flip them to non-nullable.

I actually meant for these to be in separate pull requests, but I got lazy. Pick and choose here.

In the second part I added a userspace checkKey() method so you can check keys without a memcache roundtrip.
But in doing so I noticed that the checks don't match. libmemcached does an isgraph() check on ascii keys while the php-memcached check was iscntrl().
This breaks on many strings as per the test I added.

Make ascii key check consistent with libmemcached's isgraph() check
Add test to check that they match
@rlerdorf rlerdorf changed the title Fix stubs to make them consistent for PHP 8 Fix stubs to make them consistent for PHP 8 and add a checkKey() method along with a bug fix Feb 26, 2021
@sodabrew
Copy link
Contributor

sodabrew commented Mar 2, 2021

Thanks, Rasmus! I'll review soon.

@krakjoe
Copy link
Member

krakjoe commented Jun 9, 2021

Merged, thanks.

@krakjoe krakjoe closed this Jun 9, 2021
@sodabrew
Copy link
Contributor

sodabrew commented Jun 9, 2021

Thanks @krakjoe!

@SpencerMalone
Copy link
Contributor

The iscntrl -> !isgraph is a pretty major change, memcache protocol does support non-ascii keys as outlined in https://github.com/memcached/memcached/blob/master/doc/protocol.txt by implicit statements such as:

The value of keys (and potentially other things) are "URI encoded". Since most
keys used conform to standard ASCII, this should have no effect. For keys with
less standard or binary characters, "%NN"'s are inserted to represent the
byte, ie: "n%2Cfoo" for "n,foo".

and the general key protocol definition:

Keys


Data stored by memcached is identified with the help of a key. A key
is a text string which should uniquely identify the data for clients
that are interested in storing and retrieving it. Currently the
length limit of a key is set at 250 characters (of course, normally
clients wouldn't need to use such long keys); the key must not include
control characters or whitespace.

@rlerdorf
Copy link
Contributor Author

Commented in #546

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