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

LiipDoctrineCacheBundle #1

Closed
lsmith77 opened this issue Feb 21, 2014 · 17 comments
Closed

LiipDoctrineCacheBundle #1

lsmith77 opened this issue Feb 21, 2014 · 17 comments

Comments

@lsmith77
Copy link
Member

I assume you are aware of https://github.com/liip/LiipDoctrineCacheBundle ?

@jameshalsall
Copy link

I wonder if LiipDoctrineCacheBundle should be merged into this repository so it becomes an official doctrine bundle?

LiipDoctrineCacheBundle is awesome, and seems a shame to re-invent the wheel

@lsmith77
Copy link
Member Author

i am generally open to move it to the Doctrine org .. we created it out of a need .. but we have no need to keep it in our namespace just for marketing. so if it makes it easier and more trustworthy for people to have it hear .. we are good with that. also it will likely make it easier to ensure the right people have access.

@guilhermeblanco
Copy link
Member

@lsmith77 we missed it. I didn't recall about LiipCacheBundle when we exchange emails internally (actually, none responded Fabio's emails internally).
He went ahead and implemented it and yesterday he asked me to create a repo in doctrine organization. He pushed the code and completely forgot about Liip, so sorry.

Now let's see what can we do:

  1. Remove this one and use Liip's
  2. Move Liip's to Doctrine organization and remove this one
  3. Remove Liip's and use this one
  4. Modify current code to be compatible with Liip's with minimal changes

Internally, we'd like to have Doctrine organizational support for a cache controlling as a Bundle.
This pretty much invalidates option 1. We still have 3 options to deal with. What are your thoughts?

Would like to hear from @doctrine/team-doctrine2 and @doctrine/team-symfony-integration too. Our ultimate goal is to heavily simplify Symfony\Bridge\Doctrine support.

@deeky666
Copy link
Member

I would vote for 4) because then users of LiipDoctrineCacheBundle won't have to refactor their applications too much. Another option IMO would be to "deprecate" LiipDoctrineBundle and give users time to upgrade to DoctrineCacheBundle.

@lsmith77
Copy link
Member Author

The good news is that for people to migrate to this Bundle all that should be necessary would be changing a 1) dependency, 2) their app kernel and 3) some configuration. 1) and 2) are unavoidable but 3) should ideally be kept to a minimum.

Once this Bundle is stable and provides the same features I would then deprecate Liip's Bundle.

BTW: There is a website called knpbundles.com that makes it easy to know if there is a bundle before writing a new one ;)

@deeky666
Copy link
Member

wtf sorry I clicked the wrong button :( can anyone reopen please?

@deeky666 deeky666 reopened this Feb 22, 2014
@deeky666
Copy link
Member

ah found it :D sorry. What I wanted to say is that I think deprecating the "old" bundle should be the way to go. DoctrineCacheBundle should keep the effort of "upgrading" as minimal as possible as @lsmith77 pointed out.

@guilhermeblanco
Copy link
Member

@lsmith77 I evaluated the features available as part of LiipDoctrineCacheBundle.
No matter what we try to do, at least the bundle alias needs to be modified. Right now it's liip_doctrine_cache and doctrine_cache. We cannot avoid this.
Liip uses namespaces as main section of configuration. The terminology seems wrong, because you're creating cache providers, not namespaces. Since people would have to modify the first list, they can easily modify the second.
Cache providers are pretty much the same, with an exception to file_system and php_file, which got created as filesystem and phpfile by this Bundle. We can easily change this IMHO.

Related to provider specific configuration is where we diverge. We created a schema for each driver as an specific definition, which forced users to have something like this:

doctrine_cache:
    providers:
        foo:
            type: 'file_system'
            file_system:
                extension: 'phpc'
                directory: "%kernel.cache_dir%/configurable-phpfile-provider"

Compared to LiiipDoctrineCacheBundle:

liip_doctrine_cache:
    namespaces:
        foo:
            type: 'file_system'
            extension: 'phpc'
            directory: "%kernel.cache_dir%/configurable-phpfile-provider"

Trying to converge DoctrineCacheBundle to become similar to LiipDoctrineCacheBundle, I do see a problem with this when dealing with XML driver. We would have a schema that pretty much force the ability to define ALL possible properties at root level, even though an specific driver wouldn't need/require it. Personally, I'm against trying to do this update.
Shared information, such as namespace, type, is the same. No problems at all.

LiipDoctrineCacheBundle supports aliases. We should support this too, will open a ticket for this.

Service name change. People would be force to change liip_doctrine_cache to doctrine_cache, so I wouldn't see a problem extending the change the change from liip_doctrine_cache.ns. to doctrine_cache.provider..

Custom cache types is something we haven't planned and surely we need to allow this too. WIll open another ticket for this support.

Besides these things, there's nothing else missing. Or am I wrong?

@pborreli
Copy link
Contributor

voting for 2 (Move Liip's to Doctrine organization and remove this one) and implementing the good suggestions as PR on Liip's one.

@punkeel
Copy link

punkeel commented Feb 23, 2014

imo 2 is best thing to do for now ...

@jameshalsall
Copy link

I think #2 seems the most sensible option, no one uses this bundle yet so it makes sense to move the currently more popular Liip bundle to this one

@guilhermeblanco
Copy link
Member

I'm in favor of option 4.
Reasons:

  • Liip's bundle have no tests
  • Also, there's a fundamental change required in order to properly deal with provider specific arguments, requiring us to drastically modify Liip's code if moved over here (mainly regarding XML schema support)
  • The things missing for both to be very similar are very small (provider name change, aliasing support and custom type).

Facing from a different direction (Liip being moved), we would have to write tests and also a BC break to allow proper XML schema and driver specific properties.

Also, this bundle only got published after it got tested in production at @instaclick.

@FabioBatSilva
Copy link
Member

The goal of this bundle is to replace/reduce the cache logic inside doctrine/bridge.
Even though this bundle can be used as a replacement for liip-cache-bundle this is not the real goal..

Is probably my mistake not evaluating liip before i start to work in this one.
But now this one has feature parity with liip-cache-bundle and the effort to move, refectory and write test to Liip is bigger than implementing the missing features for doctrine-cache-bundle.
I don't think its worth to spend time/effort in order to move Liip especially because its not possible to move Liip without BC breaks.

I think we should go with option 4 and people will decide by then selfs which one fits better theirs needs..

@FabioBatSilva
Copy link
Member

Please let me know if you guys have any other feature request :

Provider alias : #2
Custom cache providers : #3

@lsmith77
Copy link
Member Author

option 4) with #2 and #3 implemented sounds fine to me. I guess with alias support it could also ensure easier migration.

@guilhermeblanco
Copy link
Member

Let's consider this ticket closed then. =)

@guilhermeblanco
Copy link
Member

Besides custom cache providers, all other features have already been implemented in master. =)

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

No branches or pull requests

7 participants