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

feat: Allow for Response Body without JWT Token #320

Merged
merged 1 commit into from
Mar 30, 2017

Conversation

AlexBachmann
Copy link
Contributor

This pull request is regarding Issue #313.

My proposed change would not change to current behavior of the bundle. All it does is respecting any changes being made to the response data during the AUTHENTICATION_SUCCESS event. That is, if an event listener decides to remove the token from the data array, it will no longer be added later.

@chalasr
Copy link
Collaborator

chalasr commented Mar 30, 2017

👍
Could you have a look at the failing test? It surely ensures that the token can't be removed from the response data, that is no more relevant.

@AlexBachmann
Copy link
Contributor Author

Looking at the tests, I realized, that a better implementation would be to add the token to the response data in the constructor. This brings the implementation closer to the original JsonResponse implementation while preserving the previous behavior.

The replaceData Test now checks that the token is really removed, when the data array does not contain the token data.

@@ -28,18 +28,8 @@
*/
public function __construct($token, array $data = null)
{
$this->token = $token;
$data = is_null($data) ? [] : $data;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could $response = null be changed to $response = [] in the signature? It would remove the need for this line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@chalasr
Copy link
Collaborator

chalasr commented Mar 30, 2017

Thank you @Batch1211!

@chalasr chalasr merged commit 4b6f328 into lexik:master Mar 30, 2017
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