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

Better logged user handling #7

Merged
merged 1 commit into from
May 26, 2021
Merged

Better logged user handling #7

merged 1 commit into from
May 26, 2021

Conversation

martenb
Copy link
Contributor

@martenb martenb commented May 26, 2021

We have same issues, that is described in issue #5 , so i prepared PR, which fixes it.

@rootpd
Copy link
Owner

rootpd commented May 26, 2021

@martenb Thanks a lot for the PR. I'm having a bit harder time understanding what was the cause of the issue. Could you please describe the change? It looks the same to me, but I must be missing something.

This is how I read it:

  • The actual log() logging is only executed if identity is not null. Previously it was handled by if ($this->identity) condition.
  • Identity is available for access only if user is set and logged in. Previously it was handled by if ($user->isLoggedIn()) in setUser method.
  • User is not used in any new way.

Thanks.

@martenb
Copy link
Contributor Author

martenb commented May 26, 2021

Main problem is, that User::getIdentity() was called in beforeCompile, so if you have changed UserStorage::setNamespace() in Presenter, described in the issue, it causes that you were unable to login to app.
So only thing i have changed is, that User::getIdentity() is called after error ocurs, not in beforeComplie..

@martenb
Copy link
Contributor Author

martenb commented May 26, 2021

// if you do this in BasePresenter, you are unable to login
// not sure, why
$this->getUser()->getStorage()->setNamespace('Admin');

Now this works and correct user is logged to sentry (with corrent namespace).

@rootpd
Copy link
Owner

rootpd commented May 26, 2021

Wow, thanks a lot.

@rootpd rootpd merged commit c94fe41 into rootpd:master May 26, 2021
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