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

Add testcase for get-method overload #210

Closed
wants to merge 2 commits into from
Closed

Add testcase for get-method overload #210

wants to merge 2 commits into from

Conversation

SjonHortensius
Copy link

Test change introduce in #209 to fix #126

sodabrew pushed a commit to sodabrew/php-memcached that referenced this pull request Jan 24, 2017
sodabrew pushed a commit to sodabrew/php-memcached that referenced this pull request Jan 24, 2017
@sodabrew
Copy link
Contributor

sodabrew commented Jan 24, 2017

This presently fails with:

Warning: Declaration of test_overload_get::get($key, $cache_cb = NULL, &$cas_token = NULL)
should be compatible with Memcached::get($key, $cache_cb = NULL, $get_flags = NULL)
in tests/overload_get.php

SjonHortensius added a commit to SjonHortensius/php-memcached that referenced this pull request Jan 24, 2017
@SjonHortensius
Copy link
Author

I originally added this test to make sure php7 userland code will be able to overload Memcached::get successfully. After adding this test, dc5b22a fixed the issue, but broke this test.

I've added a commit that fixes the test by implementing the new get() signature

@sodabrew
Copy link
Contributor

Thanks for your responsiveness @SjonHortensius! I'm going to close this PR without merging, however. The test at this point is no longer checking the underlying (and unresolved) PHP
https://bugs.php.net/bug.php?id=66331 and so the effect is just to test normal class inheritance, which is basic PHP and not something we need tests for here.

The php-memcached API going forward adds a flag field to get and getMulti, with the value Memcached::GET_EXTENDED changing the methods to return an array. Because the flags field is a pass-by-value integer, it's possible to subclass or override the method using the normal PHP syntax without running into the corner case with the references.

@sodabrew sodabrew closed this Jan 24, 2017
@SjonHortensius
Copy link
Author

@sodabrew thanks for explaining. I agree with your statement, and I hope php-memcached will only implement signatures in the future which can be overloaded normally.

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.

User-flags related changes cause issues during inheritance
2 participants