Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exposing OneLogin_Saml2_Auth->getLastErrorReason() #38

Closed
Czachur opened this issue Apr 5, 2016 · 5 comments
Closed

Exposing OneLogin_Saml2_Auth->getLastErrorReason() #38

Czachur opened this issue Apr 5, 2016 · 5 comments

Comments

@Czachur
Copy link

Czachur commented Apr 5, 2016

Hey @aacotroneo ,

Would you be able to create a public method in Saml2Auth.php to expose OneLogin_Saml2_Auth->getLastErrorReason(). I can create a PR if you want.

Nathan.

@aacotroneo
Copy link
Owner

I think a PR for that would be nice (I cant test the code myself right now). We could just expose the hole object just in case... getOneLoginSaml2Auth() that returns auth?

@Czachur
Copy link
Author

Czachur commented Apr 5, 2016

That is a good suggestion!

Another idea would be to have Saml2Auth extend OneLogin_Saml2_Auth, then Saml2Auth would have access to the public methods in OneLogin_Saml2_Auth (Also meaning you could remove Saml2Auth->isAuthenticated(), Saml2Auth->login() and Saml2Auth->logout()) . Can you see any issues with this inheritance?

@aacotroneo
Copy link
Owner

Inheritance adds a little more of coupling.. if onelogin changes their api in the future, we change our implementation and our users will be safe.
I didnt expose the raw object initially to have a clean API and easy to use for regular users, but I understand that for some advanced cases is needed... Its not clean, but its needed.. I would just expose the object for advanced usage and leave the rest of the api like it is.

@pitbulk
Copy link

pitbulk commented Apr 6, 2016

Hi @aacotroneo, I try to maintain the API methods of php-saml intact, but sometimes new functionalities require adding new parameters or implement new methods.
For new releases, I will try to document what API methods changed so will be easy for you to update your part.

Best regards.

@aacotroneo
Copy link
Owner

hi @pitbulk, I know that, I was just commenting a general idea about inheritance over composition, your API has been very stable for sure.. we barely had to change anything because of a breaking change here
BTW, very nice library you've made!
Cheers!

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

No branches or pull requests

3 participants