Moving Facebook PHP SDK configuration to DI instance config

widmogrod / zf2-facebook-module

Zend Framework 2 module adding integration with Facebook php-sdk

16 stars 6 forks source link

Moving Facebook PHP SDK configuration to DI instance config #2

Closed Ocramius closed 12 years ago

Ocramius commented 12 years ago

This modification allows instantiation of Facebook multiple times. The selection of the instance alias to be used for the APP_ID header could be done after this.

widmogrod commented 12 years ago

I don't fully understand benefits for this modification, could You explain it for me?

Now, with this change, I have two places to the configure; first is FacebookBundle and second is di->instance->Facebook->parameters->config

I choose approach with the one configuration section (FacebookBundle) to separate user configuration form DI and make it:

less vulnerable for mistakes
easier to customize with diffrent environments (projects)
and in futhermore, simplify process of moving configuration to the separate file

Ocramius commented 12 years ago

In my specific case I had to have different Facebook instances with different logic. Basically, I left the Facebook object and built my own implementation(s) of all the abstract methods of the BaseFacebook class.

Also, I had to have multiple instances of the Facebook object (different apps with different rights).

This modification isn't complicating config that much.

I fully understand your doubts about it's usefulness vs. complication, but what I saw in your code is that the FacebookBundle\Module was instantiating the Facebook object. That is a wrong approach in my opinion, but you probably have a reason for it :)

I can probably do a new PR later with some more details about what I was meaning about "selecting the instance alias to be used for the APP_ID header". :)

Ocramius commented 12 years ago

Ouch, didn't notice that Github continued to track commits on my own fork :( @widmogrod merry christmas by the way :)

widmogrod commented 12 years ago

@Ocramius thank you for christmas wishes & happy new year :)

widmogrod commented 12 years ago

I push some changes, what do you think? I think it's compromis for our approaches

Ocramius commented 12 years ago

I wouldn't push them now... Too many changes to your code that are unrelated to this PR.

Did you try this module with the latest module loader impl? Because I'm not sure it would work...

Ocramius commented 12 years ago

I'm closing the PR for now :)