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

Fix when and unless return value #807

Merged
merged 2 commits into from
Apr 15, 2021
Merged

Conversation

netpok
Copy link
Contributor

@netpok netpok commented Apr 9, 2021

  • Added or updated tests
  • Documented user facing changes
  • Updated CHANGELOG.md

Changes
This PR fixes the return value validation for the when and unless methods on the Models

I'm also not sure about this whole feature as it does what the when method should do but not what it actually can.

A more realistic (and invalid) version:

    /**
     * Apply the callback's query changes if the given "value" is true.
     *
     * @template TValue
     * @template TCallbackResult
     * @template TDefaultResult
     *
     * @param TValue  $value
     * @param callable($this, TValue): TCallbackResult $callback
     * @param callable($this, TValue): TDefaultResult|null  $default
     * @return TCallbackResult|TDefaultResult|$this
     */
    public function when($value, $callback, $default = null);

Or a valid version: A more realistic (and invalid) version:

    /**
     * Apply the callback's query changes if the given "value" is true.
     *
     * @param mixed  $value
     * @param callable($this, mixed): mixed $callback
     * @param callable($this, mixed): mixed $default
     * @return mixed|$this
     */
    public function when($value, $callback, $default = null);

Breaking changes
None

@szepeviktor
Copy link
Collaborator

szepeviktor commented Apr 9, 2021

Thank you for your contribution.

Could you add a test for each method (which fail without these modification)?

@netpok
Copy link
Contributor Author

netpok commented Apr 12, 2021

Updated existing test to fail

@netpok
Copy link
Contributor Author

netpok commented Apr 15, 2021

@szepeviktor Is this test acceptable?

@szepeviktor
Copy link
Collaborator

It seems so.

@canvural canvural merged commit 5207bd9 into larastan:master Apr 15, 2021
@canvural
Copy link
Collaborator

Thank you!

@netpok netpok deleted the netpok-patch-1 branch April 15, 2021 21:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants