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

Refresh memcached.ini and set sess_binary_protocol off by default with older libmemcached #330

Merged
merged 2 commits into from
Jun 13, 2017

Conversation

remicollet
Copy link
Collaborator

1st commit is mostly cleanup, follow php practice, but needed by the second one

2nd commit is another way to (try to) improve user experience with older libmemcached version and avoid the warning about using touch (which can be confusing) => for discussion.

- remove deprecated options
- add missing memcached.sess_server_failure_limit option
- comment all options default value (only needed when not default value)
  see php.ini-production which follow this convention
@sodabrew sodabrew added this to the 3.1.0 milestone Feb 21, 2017
@sodabrew
Copy link
Contributor

Thanks! I think this makes sense for a 3.1 minor rather than a 3.0.x micro?

@remicollet
Copy link
Collaborator Author

remicollet commented Feb 21, 2017

Thanks! I think this makes sense for a 3.1 minor rather than a 3.0.x micro?

As you prefer, your the lead ;)

But for downstream distribution (I maintain the Fedora, EPEL [1] and CentOS SCL packages [2]) we have to provide a clean an working configuration. Having the current one have to be fixed to avoid the "touch" warning, so I will probably have to apply this patch there, and I don't like to diverge from upstream.

[1] https://apps.fedoraproject.org/packages/php-pecl-memcached
[2] https://wiki.centos.org/AdditionalResources/Repositories/SCL with 3.0.3 in "testing"

P.S. relying on the provided configuration is usually not enough, as this file is not updated when touch by the system admin, reason why I prefer the default value in the code.

@sodabrew sodabrew merged commit bf16783 into php-memcached-dev:master Jun 13, 2017
@sodabrew sodabrew modified the milestones: 3.0.4, 3.1.0 Jun 13, 2017
@@ -332,7 +332,11 @@ PHP_INI_BEGIN()
MEMC_SESSION_INI_ENTRY("lock_wait_max", "2000", OnUpdateLongGEZero, lock_wait_max)
MEMC_SESSION_INI_ENTRY("lock_retries", "5", OnUpdateLong, lock_retries)
MEMC_SESSION_INI_ENTRY("lock_expire", "0", OnUpdateLongGEZero, lock_expiration)
#if defined(LIBMEMCACHED_VERSION_HEX) && LIBMEMCACHED_VERSION_HEX < 0x01000018
MEMC_SESSION_INI_ENTRY("binary_protocol", "0", OnUpdateBool, binary_protocol_enabled)
Copy link
Contributor

Choose a reason for hiding this comment

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

If this isn't already mentioned in the (package.xml) changelog, could you mention it?

  • E.g. someone could upgrade memcached, not notice that the default session binary/text protocol varied based on libmemcached version, use Memcached with something that only supports text protocol(twemproxy), and then upgrade libmemcached, then see something break.
  • Or they could have libmemcached and a proxy only working with binary protocol, but I'm not aware of proxies like that.

@remicollet remicollet deleted the issue-ini branch June 13, 2017 17:09
@sodabrew sodabrew modified the milestones: 3.0.4, 3.1.0 Nov 20, 2017
@sodabrew
Copy link
Contributor

I believe I made a mistake by merging this with 3.0.4 in mind. I will branch REL3_0 and revert on that branch in order to release 3.0.4 without this change. It will remain queued for 3.1.0 along with other .ini changes.

sodabrew added a commit to sodabrew/php-memcached that referenced this pull request Nov 21, 2017
…e-ini"

This reverts commit bf16783, reversing
changes made to 23cf6c6.
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.

3 participants