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

Fix filterFields for when no specific fields are passed #35

Closed
wants to merge 3 commits into from
Closed

Fix filterFields for when no specific fields are passed #35

wants to merge 3 commits into from

Conversation

mbardelmeijer
Copy link
Contributor

Changes

This PR is open for discussion. We're running into an issue with the Authentication -> login method. This method uses the filterFields method without specific fields defined. Passing the empty array to the Arr::only method will return no fields at all, which returns in authentication failure. This PR fixes this behaviour, but unsure if this is the best way to fix this.

Checks

  • The version constant is updated in Client.php
  • The changelog is updated (when applicable)
  • The supported Cluster API version is updated in the README (when applicable)

@dvdheiden
Copy link
Collaborator

Thank you for the PR.

Actually, we could just remove the filterFields call from the login endpoint as all fields are allowed to be passed. I'm not sure if it's the best approach to use array_filter as that filters nullable fields (as we now have several nullable fields that might cause some problems), maybe when no fields are provided we should just return the source array as-is.

Besides that, I'm actually not that happy about the filterFields anyway, as it resulted in other errors before so I might give it a try to handle that differently.

@dvdheiden
Copy link
Collaborator

Did some checks, but this introduces another problem because it now filters fields that are set to null. That will reintroduce the issues with the rabbitmq_* fields. I will create a new PR for this to fix the login endpoint and will keep this in mind while working on #7.

@dvdheiden dvdheiden closed this Dec 31, 2022
@dvdheiden
Copy link
Collaborator

Thanks again for reporting this issue and putting the effort in to submit a PR.

This issue is fixed in https://github.com/CyberfusionNL/cyberfusion-cluster-api-client/releases/tag/v1.86.3 by removing the filterFields call in the login endpoint.

In the PR I provided an example: #36 (comment)

If you have any issues with the client, please let me know!

@mbardelmeijer
Copy link
Contributor Author

Thanks a lot! Doubted it as well if this was the complete fix; agreed that removing the filterFields from Login makes the most sense.

Happy new year! 🎆

@dvdheiden
Copy link
Collaborator

Happy new year! 🎊🎆

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