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

Fix #74284: Update runtime configuration page for memcached #53

Closed
wants to merge 1 commit into from

Conversation

jboulen
Copy link
Contributor

@jboulen jboulen commented Mar 2, 2020

I based my work on php-memcached/memcached.ini.

It should also fix this issue.

@@ -85,14 +139,32 @@
</row>
<row>
<entry><link linkend="ini.memcached.serializer">memcached.serializer</link></entry>
<entry>php</entry>
<entry>igbinary</entry>
Copy link
Contributor

Choose a reason for hiding this comment

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

This should continue to be php - PHP is the default. igbinary can only be used if the ini setting is changed, and the igbinary extension is installed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment on default memcached.ini indicates :

; The default is igbinary if available, then msgpack if available, then php otherwise.
;memcached.serializer = "igbinary"

So I will update this line too.

The default is igbinary if available and php otherwise.

But what default value should we display in documentation ?

Copy link
Member

Choose a reason for hiding this comment

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

I think using "php" as default, but explaining that it depends on compile time (options), might be best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can I use changelog column or it's better to let explanation in settings description ?

Copy link
Contributor

@TysonAndre TysonAndre Mar 11, 2020

Choose a reason for hiding this comment

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

I'm sorry. I was mistaken, this was right -- I didn't expect igbinary and msgpack to be the default and thought it'd be opt-in.

If igbinary is available, it's igbinary, if msgpack is available, it's msgpack, and otherwise, it's php.

https://github.com/php-memcached-dev/php-memcached/blob/v3.1.5/php_memcached_private.h#L105-L114

#if defined(HAVE_MEMCACHED_IGBINARY)
# define SERIALIZER_DEFAULT SERIALIZER_IGBINARY
# define SERIALIZER_DEFAULT_NAME "igbinary"
#elif defined(HAVE_MEMCACHED_MSGPACK)
# define SERIALIZER_DEFAULT SERIALIZER_MSGPACK
# define SERIALIZER_DEFAULT_NAME "msgpack"
#else
# define SERIALIZER_DEFAULT SERIALIZER_PHP
# define SERIALIZER_DEFAULT_NAME "php"
#endif /* HAVE_MEMCACHED_IGBINARY / HAVE_MEMCACHED_MSGPACK */

Copy link
Contributor

Choose a reason for hiding this comment

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

php-memcached-dev/php-memcached@abd0cae#diff-af2b8e0bfbc900b4a13b10a7af197df3 - It seems like igbinary's been the default if available since 1.0.1, as far as I can tell (commit was December 29th, 2009) - https://pecl.php.net/package/memcached

@cmb69
Copy link
Member

cmb69 commented Mar 11, 2020

I can't really comment on memcached, but please note that the PHP manual is supposed to document also older versions. So if some of these changes would have been introduced only in recent versions, respective changelog entries should be added (or be documented otherwise).

@jboulen
Copy link
Contributor Author

jboulen commented Mar 11, 2020

I will try to fill changelog column. Thank you for feedback.

@jboulen
Copy link
Contributor Author

jboulen commented Mar 19, 2020

I tried to update information about availability or depreciation of each variable.

If someone can review this PR, it will be great !

@cmb69
Copy link
Member

cmb69 commented Mar 19, 2020

@sodabrew, since you seem to be the current lead maintainer, could you please review?

@afilina
Copy link
Contributor

afilina commented Jan 7, 2021

Pinging @sodabrew since this PR seems to have fell through the cracks.

@sodabrew
Copy link
Contributor

sodabrew commented Jan 8, 2021

Thanks for the nudge, this is great. Needs to be rebased, I'll take care of that. I'm not a PHP committer, so this will need to be handled by someone on team PHP. But as the maintainer of php-memcached, I've added my approval!

Copy link
Contributor

@sodabrew sodabrew left a comment

Choose a reason for hiding this comment

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

To the maintainers: please do rebase/merge this, as the current maintainer of php-memcached, this looks good!

<entry>PHP_INI_ALL</entry>
<entry><!-- leave empty, this will be filled by an automatic script --></entry>
<entry>Available since memcached 0.1.0.</entry>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<entry>Available since memcached 0.1.0.</entry>
<entry>Available as of memcached 0.1.0.</entry>

Please do the following change for all other entries

<varlistentry xml:id="ini.memcached.sess-locking">
<term>
<parameter>memcached.sess_locking</parameter>
<type>integer</type>
<type>boolean</type>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<type>boolean</type>
<type>bool</type>

<varlistentry xml:id="ini.memcached.sess-consistent-hash">
<term>
<parameter>memcached.sess_consistent_hash</parameter>
<type>integer</type>
<type>boolean</type>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<type>boolean</type>
<type>bool</type>

<varlistentry xml:id="ini.memcached.sess-binary">
<term>
<parameter>memcached.sess_binary</parameter>
<type>integer</type>
<type>boolean</type>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<type>boolean</type>
<type>bool</type>

<varlistentry xml:id="ini.memcached.sess-randomize-replica-read">
<term>
<parameter>memcached.sess_randomize_replica_read</parameter>
<type>integer</type>
<type>boolean</type>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<type>boolean</type>
<type>bool</type>

<varlistentry xml:id="ini.memcached.default-connect-timeout">
<term>
<parameter>memcached.default_connect_timeout</parameter>
<type>integer</type>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<type>integer</type>
<type>int</type>
```Apply this change to everything below

Copy link
Member

@Girgias Girgias left a comment

Choose a reason for hiding this comment

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

Types should be int or bool and user as of instead of since

@cmb69
Copy link
Member

cmb69 commented Jan 9, 2021

Thank you!

@php-pulls php-pulls closed this in c2db151 Jan 9, 2021
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.

6 participants