|
Stavros Zavrakas - 2014-04-04 22:02:32
Hi Manuel,
I was working last days with your package and it is very interesting. I would like to thank you about that, because I saved a lot of time. I was trying to integrate PHP OAuth API with Laravel framework using composer and I would like to give you a feedback about some problems that I had and finally resolved and maybe can be helpful for other people. I think that it is a good idea to use namespaces for all of your packages because it is possible that the name of your package can confict with the name of another package. For example, even the name on one of my controllers can be like oauth_client_class it is going to have a conflict with your package. Instead if you use a namespace in front of your packages, let's say phpclasses almost everybody that is using your packages can be more sure that will not have a conflict.
Another problem that I noticed is that you are using a private/protected variable that is named session_started that indicates if a php session has already started or not. I' m not sure if I am able to understand exactly the logic of this, but in my case it broke the things in line 1099, where you check if(!$this->session_started). This variable was false so it was entering in the if condition but on line 1103 that you call the session_start() it was breaking because laravel had already created a session, so the php was throwing an exception: "A session had already been started - ignoring session_start()". I experienced this only at this part of code but probably can break also on other parts on other frameworks.
The last thing that I think that can be useful, is that in the redirect function on line 986, I think that it is a good idea to put after the redirection an exit or die statement. On laravel, after the instantiation of the class and the whole logic, I had extra code to execute, so on the redirection I was expecting to be redirected to asked for facebook permissions but in my case it was executing the code after the redirection.
Regards,
Stavros Zavrakas
Manuel Lemos - 2014-04-11 08:05:44 - In reply to message 1 from Stavros Zavrakas
Sorry for the delay, for some reason I did not see the notification for this topic.
As for namespaces, the problem is that they require at least PHP 5.3, therefore I cannot use them because some users are running on hosting environments that use older PHP versions.
As for sessions, you are right, the class was not checking if the sessions were already started by some other PHP code. I just added a check for that.
As for using die after redirection, I cannot do that because many applications need to do something else on the scripts even when redirection happens. The class sets the exit variable to true, so applications are instructed to exit without outputting anything after the redirection.
Stavros Zavrakas - 2014-04-14 13:04:58 - In reply to message 2 from Manuel Lemos
Thanks for the reply,
I will double check my code about the redirection.
I would like to ask if it is your plans to implement login with eventbrite ( http://www.eventbrite.com/ ).
This is their official documentation:
developer.eventbrite.com/docs/auth/
|