Skip to content

Conversation

@shahbaz-mehar
Copy link

No description provided.

Copy link

@BushraAsif BushraAsif left a comment

Choose a reason for hiding this comment

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

Changes reviewed

class AuthenticationResult extends AbstractResponse
{
/**
* @var string
Copy link
Member

Choose a reason for hiding this comment

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

Why not making enum representations of it, so you do not need to parse strings?

Copy link
Author

Choose a reason for hiding this comment

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

@emicha As we also support PHP 5.6 and do not validate responses in the SDK, do we still need an enum and to validate the values at the SDK level?

Copy link
Member

Choose a reason for hiding this comment

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

You do not need to validate the responses if you want, but provide a hint of what the possible values are, otherwise the person will need to read up the documentation, which defeats the purpose of an SDK in the first place.

Copy link
Author

Choose a reason for hiding this comment

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

@emicha I have updated the PHPDoc to add the possible values.

@emicha emicha merged commit d4849f9 into main Jan 20, 2025
1 check passed
@emicha emicha deleted the include-authentication-result-parameters branch January 20, 2025 08:31
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.

4 participants