Skip to content

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 intophp-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 intophp-memcached-dev:masterfrom
rlerdorf:master

Conversation

@rlerdorf
Copy link
Copy Markdown
Contributor

@rlerdorf rlerdorf commented Feb 25, 2021 •
edited
Loading

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
Fix stubs to make them consistent for PHP 8
a484084
@rlerdorf
Expose internal key check via checkKey() method
2481eb2 
Make ascii key check consistent with libmemcached's isgraph() check
Add test to check that they match
@rlerdorf
Add test
27cf781
@rlerdorf
Add test file to package.xml
3ba7dd5
@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
Copy Markdown
Collaborator

sodabrew commented Mar 2, 2021

Thanks, Rasmus! I'll review soon.

@krakjoe
Copy link
Copy Markdown
Member

krakjoe commented Jun 9, 2021

Merged, thanks.

@krakjoe krakjoe closed this Jun 9, 2021
@sodabrew
Copy link
Copy Markdown
Collaborator

sodabrew commented Jun 9, 2021

Thanks @krakjoe!

@SpencerMalone
Copy link
Copy Markdown
Contributor

SpencerMalone commented Oct 18, 2023

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.

@SpencerMalone SpencerMalone mentioned this pull request Oct 18, 2023
Closed
@rlerdorf
Copy link
Copy Markdown
Contributor Author

rlerdorf commented Oct 18, 2023

Commented in #546

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rlerdorf @sodabrew @krakjoe @SpencerMalone