aspendigital / fuel-doctrine2

A Doctrine2 package for the FuelPHP framework
GNU Lesser General Public License v2.1
11 stars 1 forks source link

Several huge improvements #2

Closed davispuh closed 11 years ago

davispuh commented 11 years ago

Tested with newest FuelPHP 1.6/develop and works perfectly :)

Also consider adding this package to Composer package list https://packagist.org/

It has never been so simple to add Doctrine to FuelPHP: composer install is all that's required and start using it immediately \Fuel\Doctrine::manager();

jimcottrell commented 11 years ago

Thanks. I'll check this out. This repository was kind of a test that didn't really get updated, and I switched active development to another location, so there was probably some duplication of effort. I should have been counting more on people discovering it. Anyway, our project using Fuel with Doctrine came to an end, but I'm sure there are others who can benefit from your work.

davispuh commented 11 years ago

Well, this repo is first hit in Google. And seems perfect ;)

If you see what else can be improved that would be great. Actually I'm using this some time already. I was just little improved it for my own needs (like logger). But decided to share and improved other things also. It really made much easier integration. And now I think it's so easy as there can't be easier :D Only if have to migrate from previous version then will need do search & replace Doctrine_Fuel:: with \Fuel\Doctrine::

Thank You for this awesome package :)

jimcottrell commented 11 years ago

Okay, so I've had a chance to go through your pull request, and I can't merge it directly as written, but it's inspired some changes, which I've committed to the branch for this request (pr/2).

Some issues and notes:

I'd appreciate any further thoughts you have, or if you see an obvious problem with what I've changed here, just let me know. If everything seems good though, I'll merge this to master and submit to Packagist.

Thanks for your changes!

davispuh commented 11 years ago

looks awesome :) While for me it seemed great idea about config that it can be overloaded at any level. Thus no need to duplicate some settings if you've lots of different connections which all could be used in same application it does make it bit complicated. So maybe this is better indeed.

About annotation driver it was somewhat untested experiment with customization. Sorry it was there.

yaml there wasn't intentionally, I just copied those requirements from app I'm making.

Changes to Logger was kind of a hacks I would say. Just when I started using this it didn't worked for several data types. Also I just found another issue with Logger. Take a look at last commit. I'll have to try with this to see if it have such issue.

For testing it seemed good idea to disable cache completely but I guess maybe it's not so good idea.

Documenting and explaining isn't my strong side so there you've done perfectly. Now it's much easier to understand.

I think you broke charset, because in FuelPHP it's in configuration but outside of connection. While for Doctrine DBAL it expects it to be in connection thus should copy it from above level if it's not there.

davispuh commented 11 years ago

just checked with this Logger it works perfectly, there's no such issue :) So thanks for it, nice ;)

EDIT: by issue I mean about INSERT queries..

jimcottrell commented 11 years ago

You're right, I definitely broke charset. It should be taken care of. I'm going to merge these changes to master at this point. Thanks for your contributions!

davispuh commented 11 years ago

great :) I would also suggest to add a tag so composer will pick it as version and I'm sure users would love seeing it in Packagist ;)

jimcottrell commented 11 years ago

Done and done.