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

Update RequestListener.php #265

Merged
merged 8 commits into from Oct 28, 2019
Merged

Update RequestListener.php #265

merged 8 commits into from Oct 28, 2019

Conversation

cklm
Copy link
Contributor

@cklm cklm commented Oct 22, 2019

fixes #263 and #264

Copy link
Collaborator

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

Thanks for the double-fix! This seems a more straightforward approach too.

Since we're no longer using the AuthorizationChecker, can you also remove that? You should also add an entry in the changelog too.

@cklm
Copy link
Contributor Author

cklm commented Oct 22, 2019

Added entries in changelog - we are still using the AuthorizationChecker to check, if user-handling is activated. Seems not to be an error for me.

@Jean85
Copy link
Collaborator

Jean85 commented Oct 22, 2019

Are you sure? We already have this check on the TokenStorage for that:

if ($this->tokenStorage instanceof TokenStorageInterface) {
$token = $this->tokenStorage->getToken();
}

removed service
removed AuthorizationChecker
@cklm
Copy link
Contributor Author

cklm commented Oct 22, 2019

Are you sure? We already have this check on the TokenStorage for that:

you are right - thats a double-check - removed it

changed tests to reflect removed AuthorizationChecker
@cklm cklm requested a review from Jean85 October 22, 2019 16:31
@cklm
Copy link
Contributor Author

cklm commented Oct 25, 2019

@Jean85 can you have a look here?

@Jean85 Jean85 merged commit d521013 into getsentry:master Oct 28, 2019
@cklm cklm deleted the patch-1 branch October 28, 2019 10:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undefined variable in RequestListener
2 participants