search

spatie / laravel-enum

Laravel support for spatie/enum
https://spatie.be/open-source
MIT License
341 stars 37 forks source link

Fixed validation error...I believe... #54

Closed telkins closed 4 years ago

telkins commented 4 years ago

I can write a test for this, but the basic issue is that when using EnumRule to validate, it calls asEnum, which attempts to create a new enum with the given input. If this fails an exception is thrown...or an instance of TypeError.

It's the latter scenario that I found to be causing a problem. I've simply added Error to the catch clause, which fixes it in my own tests.

Please let me know if you have any questions or concerns.

PS I know this is Hacktoberfest time and there are complaints about people spamming for t-shirts. I'm a fan of Hacktoberfest and love the t-shirts, but this isn't spam. I legit ran into the problem today and the fix appears to be quite simple. Please give me the benefit of the doubt and let me know if you're otherwise worried. Thx. 🤓

Gummibeer commented 4 years ago

Thanks for reporting and fixing! I think that we would do better to only use Throwable instead of the union list. I would still like to see a testcase/scenario that would be fixed. I assume something like an array as value or similar?

Regarding hacktoberfest: don't worry, the complaints are about spammers changing absolutely useless things in MD files. So this is definitely a legit PR! 😉

If you could change this to the throwable interface and add a basic test I will merge and release it! 🚀

telkins commented 4 years ago

Thanks for reporting and fixing! I think that we would do better to only use Throwable instead of the union list. I would still like to see a testcase/scenario that would be fixed. I assume something like an array as value or similar?

@Gummibeer Hey. Thx for the feedback. I changed to using Throwable per your suggestion...and also learned that I could/should use that instead of the union that I normally use. 🤓

I also added a very simple test. It simply tries to validate a series of values that aren't strings or integers. (I didn't add a resource type and I don't know what other types might be missing...but I got the "normal" ones covered, I think.)

Anyway, prior to this PR the test would result in an exception being thrown, so it would return neither true nor false. So, hopefully that's sufficient in demonstrating the value of this change....and making sure it doesn't regress.

Regarding hacktoberfest: don't worry, the complaints are about spammers changing absolutely useless things in MD files. So this is definitely a legit PR! 😉

Thx.

If you could change this to the throwable interface and add a basic test I will merge and release it! 🚀

Done...! 🤓

Let me know if you have further questions/concerns. TTYL...

Gummibeer commented 4 years ago

Looks good now. 🎉 Thanks for the update and fix at all. 😊 Will get it released today or tomorrow - I hope. 🙈

Gummibeer commented 4 years ago

https://github.com/spatie/laravel-enum/releases/tag/2.0.1