Skip to content

Conversation

@szepeviktor
Copy link
Contributor

@szepeviktor szepeviktor commented Jun 12, 2020

can use non bool truly condition :(

@szepeviktor szepeviktor deleted the patch-2 branch June 12, 2020 20:09
@Gummibeer
Copy link
Member

Hey, that's intentional to allow simpler and faster when($email)->email($email) taking a single time advantage of PHPs type-juggling.
Otherwise you would have to use empty() or any other comparison all the time - which you still can do but aren't enforced to.

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Jun 12, 2020

Thank you.

Robust code has strict types. But I understand that you want to make this package popular.

@Gummibeer
Copy link
Member

Under normal circumstances I would 100% agree with you! But I would also only support the latest PHP version. This package is this easy/simple and fundamental that I beliefe that this single missing type, because of it's single specific use, is acceptable.

One thing I've thought about and am open for discussion is to type-hint the ConditionalProxy $condition - because it's not the direct user-API and we could use the when() to cast the condition to a boolean before passing it into the proxy.
What do you think?

@szepeviktor
Copy link
Contributor Author

szepeviktor commented Jun 12, 2020

What do you think?

Sustainability-centric thinking does not care much about DX.

when($email) is when(\filter_var($email, \FILTER_VALIDATE_EMAIL) !== false) for me.

szepeviktor added a commit to szepeviktor/php-conditional-proxy that referenced this pull request Jun 12, 2020
@Gummibeer
Copy link
Member

That's the super right and still possible way.
But for me, as primary Laravel dev, I would validate this in a FormRequest - so later I know that $email is empty or a valid email. That's why I would be totally fine to use type-juggling in this case.
And you are also able/allowed to use your own when() method that uses strict types. So in general I'm on your side, but sometimes I prefer easier/shorter code over too strict/validated code - at least if I've validated it before and $email is possibly also a ?string method parameter.


While writing all this I've seen your PR - will check it immediately, thanks for your work! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants