Skip to content

Conversation

slashfan
Copy link
Contributor

@slashfan slashfan commented Aug 9, 2014

Hi,

First of all, let me thank you for your bundle and library, you have saved me a lot of time lately :)

As an intensive Symfony2 user, I'd like to propose this small PR to enhance the bundle by adding :

  • API key parameter validation without the need for a RuntimeException
  • default configuration for caching and logging based on common Symfony2 file structure
  • new configuration options to enable / disable services (repositories, twig extension) if you don't make use of them
  • symfony/symfony to composer dependencies

I updated the README, and there should be no BC break, although caching and logging are now enabled by default (but since doctrine cache and monolog are required in composer I don't think it's a problem).

Thanks !

@wtfzdotnet
Copy link
Member

Hi Nicolas,

First of all thanks for your kind words an contribution, it's appreciated! :-) I just want to emphasize I love the fact you also took care of the README markdown document.

I've reviewed your pull request quickly in public transport earlier :-), I'll be taking a quick look at it in a second from my desktop as I just got home.

Honestly I had my doubts about making the caching and logging enabled by default, and I'm still not sure if logging should be, however I can agree enabling caching by default would be a good idea, as it would be just following general http protocol.

Would you mind emphasizing why logging should be enabled by default?

Thanks,

Michael

@@ -14,21 +14,55 @@ wtfz_tmdb:

That's all! Fire away!

__Want to make use of default caching?__
__Default caching and loggin capabilities?__
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo loggin => logging

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, typo !

@wtfzdotnet
Copy link
Member

Thank you again for the contribution, if you could take care of the notes I've made I'll be happy to merge it in. Would you mind if I'd contact you directly about something? :-).

Thanks,

Michael

@wtfzdotnet
Copy link
Member

There also is still a reference at the bottom in README.md, towards tmdb.xml which should read repositories.xml now with the right reference.

@slashfan
Copy link
Contributor Author

Hi,

Thanks for your reply. About enabling logging by default, I tend to log every possible thing in my applications. And even more when it comes to external calls like APIs. But you're right, I'll reset the default behavior for cachind and logging as it is not everyone preference.

I'll take care of your notes and update the PR as soon as I can. And of course you can contact me :)

Nicolas

@slashfan
Copy link
Contributor Author

Hi, I just fixed the README and set back caching and logging default behavior :)

@wtfzdotnet wtfzdotnet merged commit 75162be into php-tmdb:master Aug 11, 2014
@wtfzdotnet
Copy link
Member

Thanks again, I'll contact you later about something :-)

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

Successfully merging this pull request may close these issues.

2 participants