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

Catch TypeError in tryFrom #105

Merged
merged 1 commit into from
Oct 20, 2021
Merged

Catch TypeError in tryFrom #105

merged 1 commit into from
Oct 20, 2021

Conversation

bluemmb
Copy link
Contributor

@bluemmb bluemmb commented Aug 1, 2021

I'm not sure if this is not put intentionally or it's a bug.

But using tryFrom gives the programmer safety feel to call it especially with null values until you get an exception, read the code and realize you need to add extra try or condition into your code.

I'm not sure if this is not put intentionally or it's a bug. 

But using `tryFrom` gives the programmer safety feel to call it especially with `null` values until you get an exception, read the code and realize you need to add extra try or condition into your code.
@Gummibeer
Copy link
Collaborator

Hey,
thanks for the PR. As the goal of that method is to be pre-8.1 polyfill I will have to check how the native one will behave.
I wasn't sure if Status::tryFrom([]) will be catched!? 🤔
As an array or object are just impossible value types.
So could be that we would have to add some more checks. But I will check and let you know.
Just the reason why we don't catch Throwable.

@Gummibeer
Copy link
Collaborator

Gummibeer commented Aug 4, 2021

So I've tried PHP8.1 enum tryFrom() with different types - all types are allowed except of array and object.
So this should be the goal for the current method too. I would happily welcome a PR fixing this and adding a test for all cases. Otherwise I will do it when I have time.

https://laravelplayground.com/#/?php=81

Bildschirmfoto 2021-08-04 um 08 23 47

Bildschirmfoto 2021-08-04 um 08 23 33

Bildschirmfoto 2021-08-04 um 08 23 20

Bildschirmfoto 2021-08-04 um 08 22 49

Bildschirmfoto 2021-08-04 um 08 23 05

Bildschirmfoto 2021-08-04 um 08 22 31

@bluemmb
Copy link
Contributor Author

bluemmb commented Aug 5, 2021

@Gummibeer Thanks. I'll do it 👌

@Gummibeer
Copy link
Collaborator

As this relates to my last PR #108 I would merge this in and fix the remaining things myself now.
Thanks for your work. 🥳

@Gummibeer Gummibeer self-assigned this Oct 20, 2021
@Gummibeer Gummibeer added enhancement New feature or request hacktoberfest https://hacktoberfest.digitalocean.com hacktoberfest-accepted labels Oct 20, 2021
@Gummibeer Gummibeer merged commit b014c0a into spatie:master Oct 20, 2021
@Gummibeer
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request hacktoberfest https://hacktoberfest.digitalocean.com hacktoberfest-accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants