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 CacheAdapterFactory to map interfaces to adapters #4

Merged
merged 1 commit into from
Feb 6, 2018

Conversation

schlessera
Copy link
Contributor

The logic in ObjectCache is messy. Deciding what adapter to use should be done in a factory.

The logic in `ObjectCache` is messy. Deciding what adapter to use should be done in a factory.
@felixarntz
Copy link
Owner

Again, just discovered this after working on https://github.com/felixarntz/wp-psr-cache/blob/master/src/CacheAdapter/PsrCacheAdapterFactory.php 😞

Let's compare the two approaches (although they seem rather similar). Any suggestions about mine?

@schlessera
Copy link
Contributor Author

The main difference is that I created a mapping const, whereas you used conditionals.
You went with interface, which is always better. I didn't bother, as your previous code had mostly just fixed implementations.

@schlessera
Copy link
Contributor Author

@tfrommen is right, accepting an FQCN string as interface does not make any sense here and could be removed.

@felixarntz felixarntz merged commit 0c1cbd6 into felixarntz:master Feb 6, 2018
@felixarntz
Copy link
Owner

I merged the change of using a mappings constant into the cache adapter I had implemented, including getting rid of the unnecessary checks in the foreach loop.

@schlessera schlessera deleted the use-adapter-factory branch February 6, 2018 16:35
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.

None yet

3 participants