-
-
Notifications
You must be signed in to change notification settings - Fork 462
ref(client): replace nullable client with NoOpClient #1957
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
base: 5.x
Are you sure you want to change the base?
Conversation
src/State/Hub.php
Outdated
|
|
||
| if ($client === null) { | ||
| // No point in storing breadcrumbs if the client will never send them | ||
| if ($client instanceof NullClient) { |
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.
I was thinking that maybe we can extend the ClientInterface with methods like supportsBreadcrumbs or supportAttachments to properly solve this without having to use the instanceof check.
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.
I am wondering if we can "abuse" the Options for this, we already check $options->getMaxBreadcrumbs(); for example. Setting that to 0 would fix this for de breadcrumbs. And we can add sample_rate's of 0. Not sure that is a bit too hacky.
Because even though I love this PR and the cleanup it brings I also see it now does in some cases do quite some more work to throw away later which is less than idea. But the instanceof check is also not great... guess we also need a NullHub to fix most of that?
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.
I think that the options alone wouldn't cover all cases. For attachments or check-ins we would have to introduce new options that just exist to enable the NoOpClient.
Because even though I love this PR and the cleanup it brings I also see it now does in some cases do quite some more work to throw away later which is less than idea.
Yeah totally, having a nullable client where null means "don't do heavy work" was quite convenient.
But the instanceof check is also not great... guess we also need a NullHub to fix most of that?
It would definitely help us to stop doing pretty much any work but I don't really want to add one right now because we want to get rid of the Hub in the next major anyway.
I don't really have a nice solution for this right now, maybe creating an issue and revisiting it once we got rid of hubs is the best approach, WDYT?
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.
Yeah that sounds OK. I don't have the perfect answer either right now. This is a step in the right direction so let's refine on it further later.
This PR makes the
Clientno longer nullable. This means that checks if the client are null are no longer necessary which reduces possible errors.Also it's easier to retrieve options now, the new
NoOpClientwill provide options so it's no longer necessary to check if a client exists to get configuration options.resolves PHP-47