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

storages: added PeclMemcachedStorage using pecl-memcached extension #38

Closed
wants to merge 1 commit into from

Conversation

@hrach
Copy link
Contributor

hrach commented Feb 18, 2016

PHP 7.0 has only memcached extension available.

@hrach hrach force-pushed the hrach:memcached branch 4 times, most recently from 22bb465 to d0de70f Feb 18, 2016
{
if ($this->memcache->addServer($host, $port, 1) === FALSE) {
$error = error_get_last();
throw new Nette\InvalidStateException("Memcached::addServer(): $error[message].");

This comment has been minimized.

Copy link
@milo

milo Feb 18, 2016

Member

Isn't it dangerous? It can kill app when unfortunately fail.

This comment has been minimized.

Copy link
@hrach

hrach Feb 18, 2016

Author Contributor

Well,

  • app without cache actually can kill the server, not only one request
  • this is copy & past & modify from the current MemcachedStorage.
@hrach hrach force-pushed the hrach:memcached branch 3 times, most recently from 2f929cd to ee5ace3 Feb 18, 2016
@hrach hrach force-pushed the hrach:memcached branch from ee5ace3 to c99a9f8 Feb 18, 2016
@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Feb 18, 2016

Tests are green. the question is. should we do something with the naming. (eg. renamed MemcachedStorage to PeclMemcacheStorage (note the missing D) and create deprecated class that inherits it)

@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 18, 2016

Both are PECL and both are for Memcached, how they differs?

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Feb 18, 2016

Memcached is newer, maintained and updated for PHP7, also has richer API.


Ad naming:
You have once mentined that the class name is not a name of the extension but the storage name. But in this case there is not sultion to solve it, so I suggest:

Renamed the current MemcachedStorage to PeclMemcacheStorage (note the missing D) and create deprecated class that inherits it with the old one name.

So this storage will be an exception, where the storage implementation names are based on the extension name

@JanJakes

This comment has been minimized.

Copy link

JanJakes commented Feb 18, 2016

Memcache extension is not packaged for PHP7 neither by https://www.dotdeb.org/ nor by ppa:ondrej/php. Therefore current MemcachedStorage implementation in Nette can be considered incompatible with PHP7.

@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 18, 2016

"Pecl" is really not nice, because everything is pecl…

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Feb 18, 2016

We be make a BC break, rename the old one to the MemcacheStorage, and in the new one trhow better exception if memcache exception is loaded and not the memcached.

@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 18, 2016

But there is incompatibility… http://php.net/manual/en/book.memcache.php#115666 or not?

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Feb 18, 2016

I wrote it's a BC break. Nobody claims these storages are interchangeable. Even there is a BC break in memcached 5.6 & 7.0 - they have changed the default serializer.

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Feb 18, 2016

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Feb 19, 2016

@dg, please, let's collaborate your real objections and discuss them now. I have no intention to let this open for months or years, in that case I would do an extension instead. But I strongly believe, this should be in the core/package.

@milo

This comment has been minimized.

Copy link
Member

milo commented Feb 19, 2016

BC break can be solved by prefixing key. What about to hide Memcache and Memcached into one storage?

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Feb 19, 2016

@milo I don't think it's a good idea. It's pretty common to reuse the connection returned by getConnection.

@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 19, 2016

To rename current MemcachedStorage is imho not needed, it will be removed in next Nette (for PHP 7), or it can be replaced with this storage. Currently we can use name as NewMemcachedStorage or something similar. It is like mysql & mysqli.

@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Feb 19, 2016

If it's temporary, I like PeclMemcached more. But no objections to NewMemcachedStorage. Better something than nothing. So, pick a name, I will rename it and rebase it :)

@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 19, 2016

Yes, naming things is hard.

I don't like Pecl because it says nothing about how they differ. Moreover they both are PECL.

New extension uses libMemcached, so what about LibMemcached?

@dg dg force-pushed the nette:master branch from 1bd2cab to a3e0f71 Feb 22, 2016
@dg dg closed this in a3e0f71 Feb 22, 2016
dg added a commit that referenced this pull request Feb 22, 2016
dg added a commit that referenced this pull request Feb 22, 2016
@hrach

This comment has been minimized.

Copy link
Contributor Author

hrach commented Feb 22, 2016

Thanks. 👍

@hrach hrach deleted the hrach:memcached branch Nov 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.