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

Limit cache key length #35

Open
superdav42 opened this issue Jan 20, 2019 · 3 comments · May be fixed by #165
Open

Limit cache key length #35

superdav42 opened this issue Jan 20, 2019 · 3 comments · May be fixed by #165

Comments

@superdav42
Copy link

Memcache has a key length limit of 250 characters. Problems can arise if a cache key greater than this is used. It seems memcache truncates the key so it is very possible to have collisions and unexpected behavior. Other object caches can handle very large keys so this object cache should check if a key is too long and hash it with md5 or similar to prevent problems.

@superdav42 superdav42 changed the title Limit cache key lenght Limit cache key length Feb 18, 2019
@Krinkle
Copy link

Krinkle commented Apr 20, 2024

This appears to have been fixed by Automattic for WordPress.com VIP customers in the copy of this plugin at https://github.com/Automattic/vip-go-mu-plugins/tree/develop/drop-ins/wp-memcached as part of Automattic/vip-go-mu-plugins@da8a798.

The memcached plugin on WordPress.org is now 3 years out of date. There are several changes and PHP 8.2 fixes here in the main branch, unreleased, and several more fixes in vip-go-mu-plugins that are neither commited nor released.

@WPprodigy Is there a timeline for when these are folded back in? The patches there don't look like breaking, large, or high-frequency changes so I wonder what led to the copy being preferred, or what risk there would be with committing them here instead (and updating the submodule).

In any event, in the interim, merging these two fixes and a release for these and previously merged fixes, would be great!

Krinkle added a commit to jquery/infrastructure-puppet that referenced this issue Apr 21, 2024
Follows-up 875339c, which explored apcu. Note that php-apcu in puppet
was only ever installed. The WordPress apcu object-cache.php plugin
was only tested locally on staging servers and has long since been
removed.

The problem with apcu in WordPress is that it isn't supported by
WP-CLI currently (it is unable to purge the same logical keys, it
would probably require the apcu WordPress plugin to implement some kind
of WP-CLI endpoint over HTTP to relay purges). This could be worked
around by having Puppet restart php-fpm after any WP-CLI command(s),
but there is a simpler and arguably better solution which is to use
the official memcached plugin.

The state of memcached plugins is a bit of a mess at the moment:

* https://wordpress.org/plugins/memcached/
  https://github.com/Automattic/wp-memcached/

  This is the oldest and most mature plugin, and has the benefit
  of being maintained by a first-party WordPress contributor
  (Automattic).

  Downsides:
  - Uses the older php-memcache client, instead of php-memcached.
    The latter is based on the seemingly more complete, widely used,
    and more actively maintained libmemcache (including support
    for igbinary).

  - Last release was 2 years ago. Improvements for recent WordPress
    versions (implement get_multiple) and recent PHP versions
    (fix PHP 8.2 deprecation warnings) are waiting to be released.

  - The GitHub repo has diverged from the WordPress.org SVN repo.
    The last change in the WordPress.org repo is
    https://plugins.trac.wordpress.org/changeset/2820882/memcached/trunk
    which is in fact missing from https://github.com/Automattic/wp-memcached/

    The dozens of other changes in the GitHub repo are likewise
    missing from the SVN repo.

  - The maintainers have seemingly stopped using it themselves in
    favour of two internal forks, one at
    https://github.com/Automattic/wp-cache-memcached
    and another at
    https://github.com/Automattic/vip-go-mu-plugins/tree/develop/drop-ins/wp-memcached
    Neither seems committed to unforking and folding work
    back into the wordpress.org plugin.

  - Ref Automattic/wp-memcached#35

  - Various forks and PRs exist to add php-memcached support, but
    nothing has made it into the repo to date.

* https://wordpress.org/plugins/memcached-redux/

  Forked from wp-memcached in 2012 to switch from php-memcache to
  php-memcached.

  Downsides:

  - Last release was even longer ago, 4 years ago.
  - Missing recent improvements and fixes, e.g. missing get_multple,
    and missing fixes for PHP 8.2 warnings about "Creation of dynamic
    property $cache_misses", etc.

* Neither supports automatic installation, which is probably why
  their wordpress.org stats are so unbelievably low ("300+" and "30+"
  respectively), because people just copy the file by hand rather
  than installing via wp-admin or WP-CLI.

Ref jquery/jquery-wp-content@fff52b7
Krinkle added a commit to jquery/infrastructure-puppet that referenced this issue Apr 21, 2024
Follows-up 875339c, which explored apcu. Note that php-apcu in puppet
was only ever installed. The WordPress apcu object-cache.php plugin
was only tested locally on staging servers and has long since been
removed.

The problem with apcu in WordPress is that it isn't supported by
WP-CLI currently (it is unable to purge the same logical keys, it
would probably require the apcu WordPress plugin to implement some kind
of WP-CLI endpoint over HTTP to relay purges). This could be worked
around by having Puppet restart php-fpm after any WP-CLI command(s),
but there is a simpler and arguably better solution which is to use
the official memcached plugin.

The state of memcached plugins is a bit of a mess at the moment:

* https://wordpress.org/plugins/memcached/
  https://github.com/Automattic/wp-memcached/

  This is the oldest and most mature plugin, and has the benefit
  of being maintained by a first-party WordPress contributor
  (Automattic).

  Downsides:
  - Uses the older php-memcache client, instead of php-memcached.
    The latter is based on the seemingly more complete, widely used,
    and more actively maintained libmemcache (including support
    for igbinary).

  - Last release was 2 years ago. Improvements for recent WordPress
    versions (implement get_multiple) and recent PHP versions
    (fix PHP 8.2 deprecation warnings) are waiting to be released.

  - The GitHub repo has diverged from the WordPress.org SVN repo.
    The last change in the WordPress.org repo is
    https://plugins.trac.wordpress.org/changeset/2820882/memcached/trunk
    which is in fact missing from https://github.com/Automattic/wp-memcached/

    The dozens of other changes in the GitHub repo are likewise
    missing from the SVN repo.

  - The maintainers have seemingly stopped using it themselves in
    favour of two internal forks, one at
    https://github.com/Automattic/wp-cache-memcached
    and another at
    https://github.com/Automattic/vip-go-mu-plugins/tree/develop/drop-ins/wp-memcached
    Neither seems committed to unforking and folding work
    back into the wordpress.org plugin.

  - Ref Automattic/wp-memcached#35

  - Various forks and PRs exist to add php-memcached support, but
    nothing has made it into the repo to date.

* https://wordpress.org/plugins/memcached-redux/

  Forked from wp-memcached in 2012 to switch from php-memcache to
  php-memcached.

  Downsides:

  - Last release was even longer ago, 4 years ago.
  - Missing recent improvements and fixes, e.g. missing get_multple,
    and missing fixes for PHP 8.2 warnings about "Creation of dynamic
    property $cache_misses", etc.

* Neither supports automatic installation, which is probably why
  their wordpress.org stats are so unbelievably low ("300+" and "30+"
  respectively), because people just copy the file by hand rather
  than installing via wp-admin or WP-CLI.

Ref jquery/jquery-wp-content@fff52b7
@WPprodigy
Copy link

Hi!

So the newer plugin is mainly built for the Memcached PHP extension using the libmemcached library, whereas this plugin is built for the older Memcache php extension (programmers naming things 😅).

I do agree though, it would be good to port over the normalize_key() behavior into this plugin. The way Memcache automatically strips the key after 250chars does feel rather flakey and asking for potential collisions and some very unfun debugging sessions.

@Krinkle
Copy link

Krinkle commented Apr 22, 2024

@WPprodigy Yeah, I'm aware of the difference in PECL extensions used. I believe numerous forks and PRs exist that add support for that. Are you open to that direction still, where both are supported and auto-detected together, keeping this plugin as the main plugin? (I'd favour, like you probably, to check the libmemcached-based php-memcached, first, in case both are installed for some reason.).

It seems like that would be doable without a compat break, and fairly minimal code complexity increase. Basically a handful of "if" statements (or, if we prefer composition, by invoking one of two compatible sub classes in get_mc, and taking it from there). I hope to avoid a long term split and bifurcation in code, docs, support forum, bug reports, installation improvements, code contributions, tutorials, links from blog posts etc; since the vast majority of that doesn't vary by client class.

@edneville edneville linked a pull request Aug 6, 2024 that will close this issue
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 a pull request may close this issue.

3 participants