Skip to content
This repository has been archived by the owner on Sep 27, 2019. It is now read-only.

Handle empty values a little more intelligently #20

Merged

Conversation

michaeldyrynda
Copy link
Contributor

When using an empty check or relying on a default array_filter, anything that is considered to be empty will be ignored.

This means when using numeric values that can wind up being 0, they will be interpreted as empty and stripped from the attribute.

Unfortunately, this is then passed to HipChat as an invalid attribute (one without a value label), which returns a 400 error. Furthermore, HipChat's API error messages aren't the simplest to decipher.

{
    "value": {
        "style": "lozenge-current"
    },
    "label": "Signups today"
}

The change I'm proposing will handle the empty case a little more intelligently, resulting in the expected (and valid) attribute array.

{
    "value": {
        "label": "0",
        "style": "lozenge-current"
    },
    "label": "Signups today"
}

Using trim($value) !== '' takes into account other actually empty values (empty string, null, false, etc.), so other functionality is preserved.

When using an `empty` check or relying on a default `array_filter`,
anything that is _considered_ to be empty will be ignored.

This means when using numeric values that can wind up being `0`, they
will be interpreted as empty and stripped from the attribute.

Unfortunately, this is then passed to HipChat as an invalid attribute
(one without a value label), which returns a 400 error. Furthermore,
HipChat's API error messages aren't the simplest to decipher.

```json
{
    "value": {
        "style": "lozenge-current"
    },
    "label": "Signups today"
}
```

The change I'm proposing will handle the empty case a little more
intelligently, resulting in the expected (and valid) attribute array.

```json
{
    "value": {
        "label": "0",
        "style": "lozenge-current"
    },
    "label": "Signups today"
}
```

Using `trim($value) !== ''` takes into account other actually empty
values (empty string, `null`, `false`, etc.), so other functionality is
preserved.
@pmatseykanets
Copy link
Member

Michael, thanks for the PR. It's a very good and fair point but it begs a bigger refactoring across the board not just the CardAttribute class, which I already started based on your PR.
Hopefully I'll have enough time to wrap it up soon.
Thanks again.

@michaeldyrynda
Copy link
Contributor Author

No worries, let me know if there's anything I can do to help!

@pmatseykanets pmatseykanets merged commit 3085aea into laravel-notification-channels:master Jul 6, 2018
@pmatseykanets
Copy link
Member

Sorry it took me some time to get to it. Hopefully it solves it across the board now.
Please let me know if you run into any issues.

Thanks again for your contribution!

@michaeldyrynda
Copy link
Contributor Author

Beautiful, love your work ❤️

I’m on leave from work at the moment, but will have a poke around when I’m back in a couple of weeks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants