search

mitchellvanw / laravel-doctrine

NO LONGER MAINTAINED! A Doctrine 2 implementation that melts with Laravel
MIT License
187 stars 74 forks source link

Configuration mapping for all doctrine SQL-supported engines. #31

Closed kirkbushell closed 10 years ago

kirkbushell commented 10 years ago

This is a proposal and implementation for a neater configuration setup, when mapping Laravel 4 database configuration to a Doctrine-required mapping. It also allows for easy extension should other engines such as MongoDB be required in future.

In addition, this sets the stage for proper SQLite support (something we need really badly for tests), in-memory configuration.etc.

mitchellvanw commented 10 years ago

I really like what you've done. It's great and thanks for this. There are just two small things that can be improved, the switch statement. The Speed-Dating Pattern fits great for these kind of situations. You add a specification method to the MapperInterface and loop through all of the mappers until one is "appropriate". This is how the CacheManager works. It requires just a tad more setup, but complies to the Open-Closed Principle. The second thing is I dont like to suffix my interfaces with "Interface". The name "Mapper" for the interface is better, because it forces you to specify an implementation. If there's and interface named MapperInterface it allows you to create a class named Mapper. Please let me know if you need more information about replacing the switch statement. Thanks a lot, really appreciate the work!

kirkbushell commented 10 years ago

Sweet as Mitch, making required changes now.

mitchellvanw commented 10 years ago

Awesome man! I'll have to locally test this before I can merge it in, although I trust your code and I'm sure you've done tests on your machine. In about an hour I have a radio show to present, so I'll have to test it tomorrow. Thanks a bunch

kirkbushell commented 10 years ago

haha no, don't merge in yet. I wanted to talk to youa bout that - this was just a proposal.etc. and make sure I'm on the right page. I actually want to setup a test suite now - you happy for me to do that?

mitchellvanw commented 10 years ago

@kirkbushell oh yea, sure, go ahead

kirkbushell commented 10 years ago

Okay cool - next update will have tests and then we can go from there. Question - how are you testing while doing package development in a laravel app, with dependencies.etc?

kirkbushell commented 10 years ago

@mitchellvanw this is pretty much good to go. Some testing I think is still needed, but tests are showing positive outcomes.

jonesio commented 10 years ago

+1 - This is a big improvement on current configuration method.

SQLite configuration is broken in current master ('database' should be mapped to 'path' vs. 'dbname') and this fixes that plus far better support for other database driver config options. :thumbsup: