Allowed breadcrumbs to accept empty string

[Cawllec]

Fixes #426
This fix allows breadcrumbs to accept an empty string as a valid name argument, similar to reports accepting an empty string as a valid name parameter.

[GrahamCampbell]

👎 I don't think this is a good idea. Instead, for errors that are empty, don't attempt to leave a breadcrumb.

[Cawllec]

I think it's a good idea to be able to track when errors/events have occurred previously even if they don't have definitive names.

[GrahamCampbell]

I'll send an alternative PR when I can find some time. Potentially on Tuesday. :)

[jmshal]

Presumably the notifier will catch and notify the exception so the developer is aware of the invalid breadcrumb - rather than silently ignoring it? If the empty breadcrumb was a totally valid mistake on the developer's side, not adding the breadcrumb to any potential future report could be misleading/confusing when trying to track back the breadcrumbs? Idk, just my 2 cents.

Edit: I'm dumb - I understand the PR now xD

🏃

[kattrali]

Rather than accepting empty names, if we have an empty-named report, the breadcrumb should express it.

(Error reported using NULL)

or similar. That way we expose context that the user likely doesn't realize.

[GrahamCampbell]

My alternative PR is still coming. I really don't like the idea of silently allowing breadcrumbs without content.

[Cawllec]

I think having breadcrumbs without names in the dashboard will be better than not, as it's still contextual Bugsnag data that will let people track down potential issues in their Bugsnag usage, located in Bugsnag.

[GrahamCampbell]

This is not true. null !== ''. In fact, this won't fix the issue for the person who was passing null, since is_string(null) === false.

[Cawllec]

Can we revisit this and try to get it completed?

[GrahamCampbell]

Yeh, sorry. Been really busy. Will look at it on Friday.

[kattrali]

This general approach makes sense. The benefits of delivering the breadcrumb are

1. to be consistent about the automatic behavior of breadcrumbs, like maintaining an accurate list of errors sent to Bugsnag,
2. to surface otherwise invisible issues in configuration which would otherwise be silenced or buried in debug logs, and
3. to deliver any metadata added to breadcrumbs which could provide debugging assistance. Some errors can be surfaced by breadcrumb type and metadata alone.

All that said, I thought a bit about this interface and how we could make it easier to parse when delivering otherwise broken breadcrumbs. I put in some inline comments for a preferred interface which can close this issue out.

[kattrali]

All of these cases can be condensed to the same name, "<no name>" with a validation error in the breadcrumb metadata to indicate a configuration issue. That way, the presentation is consistent and any future interface to searching/filtering breadcrumbs can aggregate on a single name value or breadcrumbs with a BreadcrumbError, etc.

• Breadcrumb name is null?
  • <no name> - ['BreadcrumbError' => 'NULL provided as the breadcrumb name']
• Breadcrumb name is empty?
  • <no name> - ['BreadcrumbError' => 'Empty string provided as the breadcrumb name']
• Breadcrumb name is not a string?
  • <no name> - ['BreadcrumbError' => 'Breadcrumb name must be a string - "Integer" provided instead']

[GrahamCampbell]

I'd still rather this raised an error if you provide an empty description, and just the exception recording case made sure that it doesn't provide an empty description.

[GrahamCampbell]

Gonna prep that PR right now. Been meaning to do it all month.

[GrahamCampbell]

The problem really lies with the fact that this:

    /**
     * Get the error name.
     *
     * @return string
     */
    public function getName()
    {
        return $this->name;
    }

in the report object can return empty string or null.

[GrahamCampbell]

Ok, what do you make of #463. :)

[kattrali]

Decided to do both - this change allows for creating breadcrumbs with no name and appends in error message in that case, and the other change sets a sensible Report name in the case that no name is set.