symfony-cmf / core-bundle

Provides some basic helpers for rendering content documents
https://cmf.symfony.com
24 stars 33 forks source link

Put SlugifierInterface into its own repository. #165

Closed dantleech closed 8 years ago

dantleech commented 9 years ago

We have discussed this a few times already. But I want to propose simply creating a new package for the Slugifier interface - symfony-cmf/api-slugifier.

The class would be caled Symfony/Cmf/Api/Slugifier/SlugifierInterface.

Maybe in the future we will find a better way, or maybe we will create more packages like this, however I think decoupling this interface from the CoreBundle is important.

dantleech commented 9 years ago

What do you guys think? @dbu @lsmith77 @WouterJ

wouterj commented 9 years ago

I like small libraries and I agree that there is some functionality in CoreBundle that would perfectly serve as a nice little library. We might end up with a utils/commons repository, which has subtree splits to still make it easy to maintain all these small libraries.

However, I'm wondering why we're creating or own slugifier implementation? Why don't we use Behat's Transliterator, Cocur Slugify or Ddd Slug (to name a few I liked from a quick research)?

dantleech commented 9 years ago

@WouterJ we don't make an implementation -- its just the interface (ok, and a `CallbackSlugifier implementation) - precisely because there are lots of other Slugifiers out there :)

dantleech commented 9 years ago

@dbu @lsmith77 what do you think? the main motivation is to be able to use the Slugifier interface without depending on the CMF CoreBundle.

dbu commented 9 years ago

Should we make it a doctrine slugifier instead? Phpcr-odm might use that for names. Am 26.04.2015 17:19 schrieb "dantleech" notifications@github.com:

@dbu https://github.com/dbu @lsmith77 https://github.com/lsmith77 what do you think? the main motivation is to be able to use the Slugifier interface without depending on the CMF CoreBundle.

— Reply to this email directly or view it on GitHub https://github.com/symfony-cmf/CoreBundle/issues/165#issuecomment-96399201 .

dantleech commented 9 years ago

@dbu not sure I understand.

The idea here is to create an API library (like PSR logger). It would contain the SluggerInterface and a callback slugifier for integrating third-party slugifiers.

For example, the RoutingAuto component is coupled to the core bundle through this dependency. (also Sulu).

dantleech commented 9 years ago

Oh now I understand. Its been a long day. I'm not sure Doctrine is a better place for this.

dbu commented 9 years ago

i am more and more convinced that we do need a slugifier interface. i just happen to need a slugifier in every second project for something or other, and its always rather unelegant.

well, i think doctrine is perceived much more general than the cmf. and i would rather have cmf bundles depend on a doctrine tool than phpcr-odm depend on a thing from the cmf repositories. and at least for phpcr-odm, a slugifier is a viable way to build document ids.

then again, what we really should have is a PSR for it. did you ever try to start one? i guess meanwhile we can start a repository either here so that we have the "thing" until the PSR is decided. let our repository both provide the interface and the callback slugifier, and if the PSR gets accepted we can make a version 2 that relies on the official interface.

lsmith77 commented 9 years ago

yeah .. lets create this repo .. it shows that we are serious about this and it might get some early adopters/

dantleech commented 9 years ago

I created: https://github.com/dantleech/api-slugifier

The only issue before moving it here is how it should be named:

dantleech commented 9 years ago

Also, see the closed PR: https://github.com/dantleech/api-slugifier/pull/1

wouterj commented 9 years ago

I think we should name it symfony-cmf/slugifier.

dantleech commented 9 years ago

@WouterJ do you not think that would create an expectation for that package to be an implementation?

wouterj commented 9 years ago

It isn't a fully API package, as it has one implementation. Furthermore, I don't think API packages have to have the API suffix (see laravel/contracts and PSR's packages for instance).

If there is a need for API in the name, my vote would go to symfony-cmf/slugifier-api. We should always use the symfony-cmf/ vendor name imo

dantleech commented 9 years ago

With PSR it is more clean-cut. There are no "real" implementations in any PSR. Whereas we could imagine a hypothetical CMF Slugifier component which implemented the CMF Slugifier API.

The Laravel repo you mentioned provides another possiblity of having "all" our interfaces in a single repository i.e. symfony-cmf/contracts or symfony-cmf/api.

dantleech commented 9 years ago

Indeed maybe the latter option would be a good starting point, the namespace is Symfony\Cmf\Api\ anyway, so if we decieded to break it out at a later stage, then we could do so without breaking peoples code.

dantleech commented 9 years ago

@lsmith77 @dbu wdyt? shall we do this, and what should the repository/package be called?

lsmith77 commented 9 years ago

it feels a bit like choosing a poison. I am not such a big fan of an interface repo. so I would prefer a single repo with our interface and our implementation(s).

dantleech commented 9 years ago

Interesting, but then I think that if we were to have a slugifier implementation in the repo, then we would be less likely to have third party implementations of the interface, and people will also be fooled into thinking that the CMF slugifier package contains an actual slugifier.

lsmith77 commented 9 years ago

why fooled? that repo would indeed contain the interfaces and implementations. it would then however also mean that we would then also put adapters to 3rd party slugifiers in there I guess? then again having 3rd party adapters in a separate repo would make it possible for us to really clean up the composer dependencies since we would then have a hard dependency on the 3rd party slugifier + a hard dependency on symfony-cmf/slugifier-implementation .. then we can require a symfony-cmf/slugifier-implementation ..

but then we would also need to keep our slugifier implemenation in a totally separate repo. to keep things simple for new users I would then have a hard dependency on that repo symfony-cmf/slugifier in symfony-cmf/symfony-cmf.

dantleech commented 9 years ago

I imagine this repository to contain only the interface and callback slugifier. I think the callback slugifier could be used to wrap most existing slugifiers without the need for an adapter.

Although maybe we should create a "default" CMF slugifier, and put that in symfony-cmf/slugifier alongside the interface and callback slugifier. We could then depend on that package without enforcing the inclusion of a third-party slugifier, whilst also allowing that possiblity. But I think at this stage we would play down the role of the interface as a general standard, but maybe that is for the best.

lsmith77 commented 9 years ago

the key is if we want to have a virtual package symfony-cmf/slugifier-implementation which any adapter or direct implementation would add as a provide similar to how we do this with phpcr implementations and jackalope transports.

wouterj commented 9 years ago

Would we really want to make this that complicated? I propose to keep this as simple as possible, have a package with an interface, a callback implementation and add some bridges (which means just extending the callback implementation) for popular slugifier libraries.

dantleech commented 9 years ago

If we can have an implementation in the slugifier package, then I think the slugifier-implementation virtual package would be unnecessary.

If we do not have a real implementation in slugifier then we would always depend on another package, and a virtual package would be beneficial. This approach also means that the user MUST find and install a slugifier implementation for things such as RoutingAutoBundle (or we depend on a potentially redundant third-party package).

dbu commented 9 years ago

lukas put this into the 1.3 milestone. do we want to do something about this?

i still feel that anything short of a slugifier PSR is not worth it to separate this into its own repo. but i will not veto if others feel they need the interface repo and the virtual implementation. it sounds the right repo organization but imho should not be limited to the cmf.

wouterj commented 8 years ago

RoutingAuto now cannot support Symfony 3 because it needs this sluggifier interface (and CoreBundle can't support Symfony 3 without dropping 2.3 support).

I propose to create a slugifier repository, put the interface and callback implementation in it and call it symfony-cmf/slugifier. This seems to be the easiest solution and it is at least better than the current situation.

dantleech commented 8 years ago

I think slugifier-api as it has no implementation and the name slugifier (to me) implies an implementation. It might also make people more likely to implement it.

dantleech commented 8 years ago

i would then also put it in the namespace Symfony\Cmf\Api -- and we can release that change in RoutingAuto 2.0

wouterj commented 8 years ago

Honestly, I don't think we can make other libraries implement it. You need to be a bigger organization or PSR in order to achieve something like that. The good news for us is that we don't need it, as the CallbackSluggifier will already be capable of handling 90% of all sluggifier implementations.

dantleech commented 8 years ago

I don't necessarily expect other libraries to implement it either, but having a slugifier package with no implementation would be strange.

dbu commented 8 years ago

okay, you convinced me. lets do this, but lets not even try to get other things to adopt this as an interface from within the cmf organization. we can try to warm up the PSR again, or see if e.g. thephpleague would be welcoming for that.

so for the name, i think symfony-cmf/slugifier sounds cleaner. if we have the CallbackSlugifier in that repo, we also provide an implementation.

dantleech commented 8 years ago

Technically CallbackSlugifier is an implementation, but it is not a solution. If I have a dependency on symfony-cmf/slugifier then I expect that package to provide an actual slugifier. If I see symfony-cmf/slugifier and foobar/slugifier in the same repo, then I think that somebody has done something wrong.

If you use slugifier-api then it is more self-explanatory I think. The callback-slugifier is a "utitility" in the samr way that the logger psr has a NullLogger implementation.

dbu commented 8 years ago

if we want to do this for 1.3, it has to happen very soon. we want to tag. but after that we start with 2.0 anyways, to upgrade to symfony 3. maybe that is the better spot to do it.

wouterj commented 8 years ago

I think we should simply vote for the name, as I have a feeling we will never agree with each other on slugifier vs slugifier-api.

I think we can skip this to 2.0, but it feels weird to have a component not supporting Symfony 3 just because of relying on this interface.

lsmith77 commented 8 years ago

Either name is fine for me.

dantleech commented 8 years ago

I am still slugifier-api for reasons explained above. Does anybody else see it as an issue that the package is called slugifier and that there is and never will be an implementation in it? Otherwise hapy to go with the vote.

dbu commented 8 years ago

i think the name is not that important. it should (some point in the future) be a PSR or something on the same level. like thephpleague or something similar with a reach outside of symfony cmf. go with slugifier-api if you prefer that.