User-flags related changes cause issues during inheritance

[mkoppanen]

More information:

symfony/symfony#9797

https://bugs.php.net/bug.php?id=66331

[mkoppanen]

Trying to figure out a fix for this before next beta.

[mkoppanen]

After some discussions on IRC it doesn't look like there is an easy fix for this. //cc @poison

[quipo]

Hi Mikko,

we stumbled onto this bug whilst testing PHP 5.5.8. The latest stable php-memcached version (2.1.0) does not compile with PHP 5.5, and 2.2.0b1 has this inheritance bug, so we're kind of stuck between a rock and a hard place.

What is the new functionality actually used for? Do you have use cases that explain it? How is #125 breaking the tests?

[mkoppanen]

The userflags parameter was added to to be able to set what the name says, numeric flags on keys. This allows for example versioning keys between application releases. The problem (if I remember correctly) is that if cas tokens is not prefer_ref the following signature would not work:

get ($key, null, null, $user_flags)

due to only variables being able to be passed by reference. user flags have not been released in a stable release yet so there is still a bit wiggle room on how to fix this. However the inherent issue is actually with PHP rather than memcached extension.

[poison]

The problem is that there's no real function overloading in PHP. We tried to solve this by detecting if parameters are passed by reference or if they are null, but I didn't know that this was causing issues with inheritance. Maybe we should add a ./configure parameter with --enable-user-flags which is currently default off until this PHP bug is fixed. What do you think @mkoppanen?

Next to that I'm wondering what the real purpose is of extending the memcached connector (as opposed to encapsulating it). But this is ofcourse beyond the scope of this discussion.

[mkoppanen]

I am not a huge fan of changing signature based on configuration parameter as it makes writing portable code quite hard. One option that might be a work-around is to make cas_tokens a ref, which would mean that null wouldn't work but ($cas_tokens = null) would. Another option I can see is making additional call "getUserFlags", which would retrieve user flags from previous call. Neither solution seems ideal to me, but the notice might cause problems for users.

As for your comment on extending the connector, when you create mocks in PHPUnit it will trigger this notice.

[mkoppanen]

I think I would be leaning on setting cas_tokens to ref instead prefer_ref and changing it in the future when PHP compiler is fixed. The downside of this is that all calls that use userflags would also have cas set on. Not sure how big of an issue this is.

[poison]

I'm not sure what the impact of enabling CAS on every operation is, but I think it gives extra cpu cycles to the memcached daemon. I'll try to verify this. If that's the case I'm not a huge fan of this, unless we add the ability to disable CAS on the connector completely (eg.: BEHAVIOUR_IGNORE_CAS). We do start our memcached servers with -C (= disable the use of CAS (and reduce the per-item size by 8 bytes)).

[mkoppanen]

Yes, I think additional cycles are used on both client and server side. There is no ability to disable CAS completely. In your use-case, are user flags usually the same for all items? If they are the following api might be possibly ok:

Add Memcached::OPT_USER_FLAGS, which can be set/get via the normal setOption/getOption and add additional call getLastUserFlags, which returns user flags from the latest call. Does this sound workable or does it make the use more awkward?

[mkoppanen]

So, while this causes issues to symfony (phpunit?) folks and possible misc users I am inclined to carry on with next release with the functionality as is. The inherent problem here is the PHP bug 66331 and I don't think we should change our API to accommodate for this. Opinions?

[quipo]

Whilst people might still hit this issue when extending memcache, it's easy to work around it by using composition instead, so not a big deal. Unless you need to mock the class with PHPUnit, of course.

[mkoppanen]

Agreed, I will leave this issue open as a reference when people come asking

[mevdschee]

Yes, the inherent problem here is the PHP bug 66331. But this problem is introduced in 2.2.0 and it did not exist in 2.1.0. I think you should have not broken your API between these (minor) versions. Why did you make this braking change knowing you would impact mocking and extending? Can you still fix this? I truly respect your hard work on this awesome plugin, but I feel you made a wrong judgement here.

[mkoppanen]

As for why this change was made: The change was made to accommodate user flags related functionality, which is somewhat important feature.

Can we fix it: this is something that should be fixed in PHP. I haven't really had much time recently to look into the bug more deeply but it is something that I plan to do. If your definition of a fix means changing the API that was released in 2.2.0, then the answer is no.

This functionality was added in 2.2.0b1 (released in 2013-11-25) and as you can see from the comments in this bug very few people actually participated in the discussion. Moving forward, if you actually test the beta versions and release candidates and provide your opinions at that stage then you can actually have a positive impact on the project.

[mevdschee]

Sorry I did not beta test your releases, please do not blame me for ignorance.

My contribution to the community is to provide a debugger for your memcached extension for the Symfony framework (which was installed 218 times today). I hope you also repect that.

In that process, users have found these warnings. During my process of solving those I have also found some minor issues in your documentation. You can find the corrected PHP API version(s) in the repo:

https://github.com/LeaseWeb/LswMemcacheBundle/blob/master/Resources/codegen/memcached-2.2.0/memcached-api.php

Thank you for your reaction and please keep up the good work. I hope the (PHP) bug gets resolved quickly.

[mevdschee]

@mkoppanen: Your braking and blaming reminds me of the Kay Sievers incident:

http://article.gmane.org/gmane.linux.kernel/381948

Please fix it.

[mkoppanen]

@mevdschee,

I am not blaming anyone, rather just pointing out the facts. Development of this extension is an open process and we push out beta and release candidate versions to get feedback on features and bug reports so that we can provide the best possible stable version. If our users don't test these versions then we are destined to have issues like this in stable versions.

I have already reviewed the suggestion to change the arg info earlier and that causes undesirable behaviour (unable to use user-flags without turning on CAS). Hence the only proper fix I see for this is to fix the PHP bug 66331. This is not ideal but if anyone has better suggestions I am all ears.

And thank you for the documentation updates, I will update the api documentation and the PHP documentation as soon as my work load gets a bit lighter.

[mevdschee]

Without turning on CAS? Where is that CAS argument?

memcached_get in libmemcached (from: https://github.com/trondn/libmemcached/blob/master/docs/memcached_get.pod):

char * memcached_get (memcached_st *ptr, const char *key, size_t key_length, size_t *value_length, uint32_t *flags, memcached_return_t *error);

Memcached::get signature in php-memcached (from: https://github.com/php-memcached-dev/php-memcached/blob/master/php_memcached.c):

ZEND_BEGIN_ARG_INFO_EX(arginfo_get, 0, 0, 1)
ZEND_ARG_INFO(0, key)
ZEND_ARG_INFO(0, cache_cb)
ZEND_ARG_INFO(2, cas_token)
ZEND_ARG_INFO(1, udf_flags)
ZEND_END_ARG_INFO()

I would have tried to copy the signature from the C code.

I would have removed the size_t arguments (like you did), because PHP strings have a length. Also the error can be turned stored elsewhere to be retrieved with Memcached::error(), like in MySQLi, in the case of an error the function should return "false".

You added the "cas_token" argument. From the docs (http://docs.libmemcached.org/memcached_cas.html) I read that "You can get the cas value of a result by calling memcached_result_cas()". I would make a similar function "Memcached::result_cas()" and would avoid to use arguments as return variables when libmemcached also does not do so.

So that is how I would convert libmemcached signatures into PHP functions, specifically this one.

My take: I think you try to fit a little too much different cases in one function call. You probably realized this yourself as well, but you found this (clever but not yet properly working) workaround in PHP and decided to use it.

So there is the optional cas_token argument and the optional flags. Probably reversing the order would work right? Because the last argument can be an optional reference, or not? Is this the solution you are looking for? Or you can split this into two functions. One with and one without cas. Probably you already considered that.

Thank you for your time and efforts.

[mkoppanen]

Hello,

the extension has been around since 2009 and much of the API has grown organically, some of it accommodating changes in libmemcached and some through patches from users requiring specific functionality. There are quite a few areas in the code that I don't understand so well myself.

While refactoring or rewriting the API seems like an easy solution, this again would break backwards compatibility break to our users. I don't have enough free time at the moment to rewrite the extension, the documentation and support our users during the migration. However, full rewrite is something that I am keen on doing and already prototyped a version earlier when I had more time: https://github.com/mkoppanen/php-memc-lite

The specific problem at hand here is not just optional reference argument but rather detecting whether user passed a variable or a null. In the case of memcached_get we use a wrapper function "php_memc_get_impl", which takes care of get, getByKey etc. Passing a variable as cas_token will turn on cas using memcached_behavior (http://docs.libmemcached.org/memcached_behavior.html), which causes memcached_fetch_result return structure to contain the cas_token.

As for "Probably reversing the order would work right? Because the last argument can be an optional reference, or not?". Yes, this would work and also break the backwards compatibility. Given the choice between strict error due to a PHP bug or backwards compatibility break I would rather choose the former and focus on getting the actual bug fixed.

[mevdschee]

Thank you for the thorough explanantion.

Do you realize you already made a breaking change between version 2.1.0 and 2.2.0? Adding function arguments is a breaking change since the function signatures no longer match on extending classes. I am not complaining about this, but it invalidates your reasoning.

[mkoppanen]

Hello,

first of all, there is no need for the condescending tone. In this scenario one might argue whether a concrete class has any other public contract towards the user except that calling code will not need modifications when a minor version changes. Whether extending a concrete implementation (vs interface or abstract class) is part of this contract or not is negotiable in my opinion.

[hanikesn]

Hi this is still an issue. For example the widely used LswMemcacheBundle (https://packagist.org/packages/leaseweb/memcache-bundle) migrated to the memcache extension, because of that.

[digitalprecision]

@hanikesn I saw that in their git repo, seems lame to switch APIs just to hack by this issue. Until this is resolved upstream (here), I will add a php version check in my code. This should be fixed asap.

[mkoppanen]

Looks like this has not been actioned in upstream (https://bugs.php.net/bug.php?id=66331) and is still causing problems in PHP7. I think the sanest approach is to remove the user flags for now and figure out a cleaner way to do it.

Even though I am opposed to hacking around things that are actual bugs in PHP it seems that this is causing issues on our users.

Maybe someone with more experience on the engine could weigh on this? //cc @krakjoe @rlerdorf

[SjonHortensius]

@mkoppanen I agree, it's too bad PHP doesn't fix their bug, but with the release of PHP7 this issue is becoming a bigger problem. Do you think your previous suggestion of creating an additional getUserFlags() is still an option?

[mkoppanen]

Looks like even removing user flags does not help with PHP7. I opened a PR against PHP to get this fixed. Let's see how it goes.

[mkoppanen]

Looks like PR #209 sorts out issues with user flags + inheritance. Does the API seem reasonable?

[SjonHortensius]

Your change and new API looks fine to me. However, it doesn't seem to fix compatibility with php7; compiling the php7 branch, and overloading the get() method still results in an error for me. I'll add a testcase so you can confirm.

The error no longer mentions the user-flags btw.

[mkoppanen]

@SjonHortensius,

see php/php-src#1729 PR. Looks like the behaviour is expected. This would mean refactoring CAS parameter out as well or changing it to by reference

[SjonHortensius]

Ah, you're right. I forgot about that one. Let's hope a php-core dev can shed some light on that issue

[mkoppanen]

Resolved in #211, I created a new API for php-memcached. get and getMulti now take a flag (Memcached::GET_EXTENDED), which allows to return both user flags and cas tokens. I am merging #211 shortly (as soon as the merge build passes)

[jakzal]

#211 fixed the problem. I just tried it on PHP7. Any chance to port the fix back to 2.x?

[jakzal]

@sodabrew would you mind explaining why's this issue closed? No chance of backporting the fix?

[sodabrew]

I'm not planning to make any further php-memcached 2.x releases for PHP 5.x, so if the issue is resolved for php-memcached 3.x (unreleased) for PHP 7.x going forward, so I would consider this resolved.