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

Breadcrumbs #323

Merged
merged 2 commits into from
Aug 4, 2016
Merged

Breadcrumbs #323

merged 2 commits into from
Aug 4, 2016

Conversation

GrahamCampbell
Copy link
Contributor

No description provided.

@GrahamCampbell GrahamCampbell changed the title [WIP] Breadcrums [WIP] Breadcrumbs Aug 2, 2016
@GrahamCampbell GrahamCampbell changed the title [WIP] Breadcrumbs Breadcrumbs Aug 2, 2016
@GrahamCampbell
Copy link
Contributor Author

Ok, this is ready for review now.

@kattrali
Copy link
Contributor

kattrali commented Aug 4, 2016

This looks great. ⭐ After going over the interface a bit, I think there is one usability enhancement we could make - if a user specifies an invalid breadcrumb type, rather than throwing, we should convert it automatically to the manual type, perhaps with a warning. That way there isn’t a way for it to be missed in production if the breadcrumb isn’t encountered during development. I opened a similar issue for JS. This should probably be the rule where there are no enumerated types.

Other than that, I think its ready to go.

@GrahamCampbell
Copy link
Contributor Author

Ok, sure. PHP doesn't have a way to do a "warning" as such. We could just do the error_log thing that we do when a guzzle request fails I guess. I think an exception would be fine though, since it's a usage error rather than an error the user couldn't avoid.

@kattrali
Copy link
Contributor

kattrali commented Aug 4, 2016

Makes sense, error_log could be a sensible choice. Its a usage error but probably not the kind we should throw an exception for since we can recover for it. It seems plausible to imagine a scenario where some error is happening so you add a breadcrumb with a mispelled type without checking it thoroughly because things are 🔥🔥.

@GrahamCampbell
Copy link
Contributor Author

That said, if someone mistypes a constant there, they'll get a fatal error, so if we only document sending through constants, it can't really go wrong?

@GrahamCampbell
Copy link
Contributor Author

Going to mimic what I did for if the name was too long and auto-fix it within the client, and keep the strict checks in the breadcrum object constructor.

@kattrali kattrali merged commit 375d358 into develop Aug 4, 2016
@kattrali
Copy link
Contributor

kattrali commented Aug 4, 2016

🚀

@kattrali kattrali deleted the breadcrums branch August 4, 2016 21:09
@tillkruss
Copy link

tillkruss commented Aug 7, 2016

Nice work! Will there be a Laravel middleware as well that leaves breadcrumbs automatically? 😃

@GrahamCampbell
Copy link
Contributor Author

Not a middleware, since we already record the request and user another way. We use breadcrums in laravel to record events fired. We also record logs as breadcrumbs.

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.

3 participants