Skip to content

Conversation

@rlerdorf
Copy link
Contributor

@rlerdorf rlerdorf commented Oct 20, 2023

libmemcached's key verification is only done if OPT_VERIFY_KEY is enabled. We should honour this setting in the PHP extension as well and not do it automatically.
This addresses #546

Note that I am still not completely sold on making this change.

Copy link
Contributor

@SpencerMalone SpencerMalone left a comment

Choose a reason for hiding this comment

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

Implementation wise, this LGTM. In terms of:

Note that I am still not completely sold on making this change.

I wanna note that as an impacted user: explicitly documenting previously the breaking change and leaving it in place, or making us default to enabling MEMCACHED_BEHAVIOR_VERIFY_KEY and calling out that change are both 1000% acceptable to me! As long as I have a heads up that there are key requirement changes when I upgrade, I think it's totally acceptable, the only thing that is scary is upgrading and finding a new requirement without knowing what the requirements are or why keys started failing.


static
zend_bool s_memc_valid_key_ascii(zend_string *key)
zend_bool s_memc_valid_key_ascii(zend_string *key, uint64_t verify_key)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I was going to make this suggestion, but then I realized the setting is defined as 64-bits. So it goes!

REGISTER_MEMC_CLASS_CONST_LONG(OPT_VERIFY_KEY, MEMCACHED_BEHAVIOR_VERIFY_KEY);

Suggested change
zend_bool s_memc_valid_key_ascii(zend_string *key, uint64_t verify_key)
zend_bool s_memc_valid_key_ascii(zend_string *key, zend_bool verify_key)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is why I made it a uint64_t, but perhaps we should flip that one to a REGISTER_MEMC_CLASS_CONST_BOOL ?

@m6w6 m6w6 merged commit d15c2e2 into php-memcached-dev:master Sep 26, 2024
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