-
Notifications
You must be signed in to change notification settings - Fork 12
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: Bootstrapping #44
Conversation
…ing without required parameters when fetching disabled
} catch (Exception $exception) { | ||
// empty catch, $data does not exist and will be handled below | ||
} | ||
$data ??= $this->getBootstrappedResponse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, I have not seen that operator before!
Would it make sense to validate if it is anything more than an empty string?
What if it is an empty list of feature toggles?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Empty string couldn't pass here, an exception would be thrown somewhere in the providers. Empty feature toggle list is a valid response, no? For example, if your unleash server goes down you want to play it safe and don't let anyone see a list of any flag guarded features.
I can add some validator that it at least is in valid format to fail early, but I think we shouldn't check for more than the data having a "features" key and it being an array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I think we shouldn't check for more than the data having a "features" key and it being an array.
Agreed!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ivarconr I added the check in the latest commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job.
closes #43
Implemented according to specifications - if the request to unleash api fails it falls back to a bootstrap provider (if present).
Also added a configuration parameter to disable fetching the api at all in which case these things are disabled:
If fetching is disabled and no bootstrap is provided, an exception is thrown.