FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Fix stubs to make them consistent for PHP 8 and add a checkKey() method along with a bug fix by rlerdorf · Pull Request #479 · php-memcached-dev/php-memcached · GitHub

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

Closed
rlerdorf wants to merge 4 commits into
php-memcached-dev:masterfrom
rlerdorf:master
Closed

Fix stubs to make them consistent for PHP 8 and add a checkKey() method along with a bug fix#479
rlerdorf wants to merge 4 commits into
php-memcached-dev:masterfrom
rlerdorf:master

Conversation

rlerdorf commented Feb 25, 2021
edited
Loading

Copy link
Copy Markdown
Contributor

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.

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 commented Mar 2, 2021

Copy link
Copy Markdown
Collaborator

Thanks, Rasmus! I'll review soon.

krakjoe commented Jun 9, 2021

Copy link
Copy Markdown
Member

Merged, thanks.

krakjoe closed this Jun 9, 2021

sodabrew commented Jun 9, 2021

Copy link
Copy Markdown
Collaborator

Thanks @krakjoe!

Copy link
Copy Markdown
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.

Copy link
Copy Markdown
Contributor Author

Commented in #546

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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


Back | FazBrowse Home | New Git URL