Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Fixing the Cache Storage Factory Adapter Factory #2719

Closed

Conversation

SocalNick
Copy link
Contributor

@marc-mabe

If $options is set, it should be passed to the plugin manager get function or else adapters like Memcached will not properly set servers. Memcached adapter only sets servers in the constructor so they cannot be added after.

…ction or else adapters like Memcached will not properly set servers. Memcached adapter only sets servers in the constructor so they cannot be added after.
@SocalNick
Copy link
Contributor Author

Sorry - I couldn't figure a way to add a test. The only thing I know for sure affected by this is Memcached. It MUST have servers passed in via constructor $options: https://github.com/zendframework/zf2/blob/master/library/Zend/Cache/Storage/Adapter/Memcached.php#L86

@marc-mabe
Copy link
Member

Damn you are right but the StorageInterface doesn't define a constructor. that means you can't be sure that options will be handled on constructor.

@weierophinney
Copy link
Member

@marc-mabe and @SocalNick -- perhaps we need a default AbstractFactory attached that handles all but the Memcached adapter; that factory would then instantiate and call setOptions() with the passed options. Thoughts?

@ghost ghost assigned weierophinney Oct 10, 2012
@marc-mabe
Copy link
Member

@weierophinney In my mind the StorageFactory works well instantiating the storage and calls setOptions()
The Memcached adapter could be fixed by configuring servers within setOptions() if servers are given.
Given servers as options and possible already configured servers could be merged together.

@SocalNick
Copy link
Contributor Author

All the Storage Adapters extend AbstractAdapter, which implements public
function __construct($options = null) so I think it's safe to add the
constructor to the interface. This would be quick and correct, but it is
also true that the Memcached storage adapter should be fixed.

If the servers can be changed with setOptions, maybe we should lazily
instantiate the MemcachedResource. Then if you try to setOptions after the
MemcachedResource is instantiated, we throw an exception. Just thinking out
loud on this bit...

-Nick

Nicholas Calugar
http://socalnick.github.com

On Wed, Oct 10, 2012 at 10:14 AM, Marc Bennewitz
notifications@github.comwrote:

@weierophinney https://github.com/weierophinney In my mind the
StorageFactory works well instantiating the storage and calls setOptions()
The Memcache adapter could be fixed by configuring servers within
setOptions() if servers are given.
Given servers as options and possible already configured servers could be
merged together.


Reply to this email directly or view it on GitHubhttps://github.com//pull/2719#issuecomment-9311511.

@marc-mabe
Copy link
Member

@SocalNick I don't think it's a good idea defining any of the magic methods into interfaces

@SocalNick
Copy link
Contributor Author

Can we decide on a way to proceed? I'd really like to see this fixed in
2.0.3...what do you think of @weierophinney suggestion for separate
factories?

Nicholas Calugar
http://socalnick.github.com

On Wed, Oct 10, 2012 at 1:06 PM, Marc Bennewitz notifications@github.comwrote:

@SocalNick https://github.com/SocalNick I don't think it's a good idea
defining any of the magic methods into interfaces


Reply to this email directly or view it on GitHubhttps://github.com//pull/2719#issuecomment-9317522.

@marc-mabe
Copy link
Member

@SocalNick: I'll process on Monday fixing the memcached adapter

@marc-mabe
Copy link
Member

Should be fixed by #2770

@weierophinney
Copy link
Member

@SocalNick I believe this is now no longer necessary, as the Memcached resource is now only instantiated on first use.

@SocalNick
Copy link
Contributor Author

Yes - thx for getting this fixed in #2770!!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants