upgrade to new Bundle standards

symfony-cmf / search-bundle

UNMAINTAINED - search Bundle for the Symfony CMF project integrating with LiipSearchBundle

http://cmf.symfony.com/

8 stars 10 forks source link

upgrade to new Bundle standards #10

Closed lsmith77 closed 11 years ago

lsmith77 commented 11 years ago

[x] General Bundle Standards
[x] > File Naming
[x] > Interface Naming
[x] > Dependency Container Configuration
[x] Meta: README, CHANGELOG, etc
[x] Persistence
[x] Configuration, Files and Formats
[x] Testing Component Integration

marekkalnik commented 11 years ago

I'm working on it.

wouterj commented 11 years ago

I've checked the "Dependency Container Configuration", it's correct.

However, the Configuration is not correct. routing.xml should be placed in a routing directory and should be renamed, Also the controllers, PhpcrSearchController should become Phpcr\SearchController, shouldn't it?

At last, the LICENSE file needs an update

marekkalnik commented 11 years ago

I'm working on README, Testing integration and updating composer.json - all dependencies are not listed.

Interfaces are OK - there are none ;)

marekkalnik commented 11 years ago

@WouterJ IMHO, the Controller name is ok. I don't see how a subnamespace would help things. If anything - I would rename the bundle to PhpcrSearchBundle and the Controller to SearchController, but I think that the effort is not worth the gain.

wouterj commented 11 years ago

If anything - I would rename the bundle to PhpcrSearchBundle and the Controller to SearchController, but I think that the effort is not worth the gain.

In the future, we planned supporting other database layers aswell. So this change is not correct. And that's why I suggested that namespace.

But if we do not use that namespace, the classname is still wrong. It should become SearchPhpcrController, but then again, I think we need namespaces. /cc @dbu @dantleech

However, :+1: for working on this!

wouterj commented 11 years ago

btw, the README is up to date afaics?

marekkalnik commented 11 years ago

Nope, the deps are not up to date, and the LICENSE should be in Resources/meta. I guess the README template should be updated to reflect this.

marekkalnik commented 11 years ago

WIP: https://github.com/marekkalnik/SearchBundle/tree/update-standards

dbu commented 11 years ago

i guess the proper thing according to our code style indeed would be the Phpcr sub namespace for the controller.

note that imo in the longer run we should refactor the LiipSearchBundle to have a search service so that we only ever need 1 controller, and then simply add the phpcr search provider (along with any other providers we come up with) into the LiipSearchBundle and drop this bundle.

so honestly to me the CmfSearchBundle is kind of less important. its good to clean it up but we don't need to be perfectionist.

marekkalnik commented 11 years ago

Yup, I was thinking the same - that the controller should be rather replaced by a service. I'll add a test this evening and I pass on to another bundle.