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

Return validated value #280

Open
dbalabka opened this issue Oct 30, 2022 · 4 comments
Open

Return validated value #280

dbalabka opened this issue Oct 30, 2022 · 4 comments

Comments

@dbalabka
Copy link

dbalabka commented Oct 30, 2022

I've faced the fact that it is required to create temporary variables to validate values:

$firstName = getParam('firstName');
$lastName = getParam('lastName');
Assert::notNull($firstName);
Assert::notNull($lastName);

createCustomer(
    $firstName,
    $lastName
);

We can rid of temporary variables if the assertion function returns a valid value:

createCustomer(
    Assert::notNull(getParam('firstName')),
    Assert::notNull(getParam('lastName'))
);

For some of the assertion functions, we can simply return the valid value. The changes should be BC-safe.

Possible implementation: #281
Psalm support demonstration: https://psalm.dev/r/a439359976

References:

  1. Similar Java implementation: https://commons.apache.org/proper/commons-lang/javadocs/api-3.1/org/apache/commons/lang3/Validate.html#notNull(T)
dbalabka added a commit to dbalabka/assert that referenced this issue Oct 30, 2022
@frankdekker
Copy link

I'm looking for this feature as well, and this would be helpful for every method. Your example is precisely my use case. The fluent notation makes it much more cleaner to use. Your current PR is only the notNull method. Is your intention to add this to all methods? Otherwise I could also extend on this PR.

@dbalabka
Copy link
Author

@frankdekker, I will add support for other methods as well if the maintainer accepts that such a feature is required. We should wait for @webmozart's reply.

@shadowhand
Copy link
Collaborator

shadowhand commented Oct 15, 2024

I think that Assert should be used inside the createCustomer method:

public function createCustomer(mixed $firstName, mixed $lastName): void
{
    Assert::stringNotEmpty($firstName);
    Assert::stringNotEmpty($lastName);

    // And then database write, or whatever
}

The usage suggested by OP promotes using Assert (and thus, Throwable) as a validation tool, which is not how this package was intended to be used. Throws are far too costly in PHP to be used for validation.

@dbalabka
Copy link
Author

@shadowhand , that is an interesting idea. Unfortunately, the createCustomer method is a part of the library, and there is no easy way to extend it. Furthermore, createCustomer is strictly typed and does not allow nullable values to be passed as a parameter. Therefore, the only way to make static analysis tools happy is to add type guards before calling the method.

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

No branches or pull requests

3 participants