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

Allow casts with a return type of static or this to reference themselves #1103

Merged
merged 2 commits into from
Dec 4, 2020

Conversation

hailwood
Copy link
Contributor

@hailwood hailwood commented Dec 2, 2020

Right now a cast cannot specify that it returns static or this.
Allowing so would mean a new cast can extend the old one without having to redefine the get method (e.g. an Enum)
Presently if you specify static/this in the return type then those values are literally used which makes a property look like it returns an instance of the model itself.

@mfn
Copy link
Collaborator

mfn commented Dec 2, 2020

Can you please add a test showing the behavour change? Thank you!

@hailwood
Copy link
Contributor Author

hailwood commented Dec 2, 2020

Thanks for suggesting the tests @mfn, it made me realise a bug in my code :)

I've added hopefully thorough tests for all the different cases.
I also fixed an unrelated issue in another test due to a deprecated phpunit method being used.

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR LGTM!

However, can you please undo the unrelated phpunit changes as outlined?

Thank you!

tests/Console/EloquentCommandTest.php Outdated Show resolved Hide resolved
Supports

```php
/**
     * @param $model
     * @param string $key
     * @param $value
     * @param array $attributes
     * @return static
     */
    public function get($model, string $key, $value, array $attributes)
    {
        return new static($value);
    }
```

or

```php
/**
     * @param $model
     * @param string $key
     * @param $value
     * @param array $attributes
     * @return $this
     */
    public function get($model, string $key, $value, array $attributes)
    {
        return new static($value);
    }
```
@hailwood
Copy link
Contributor Author

hailwood commented Dec 3, 2020

@mfn All done :)

@mfn
Copy link
Collaborator

mfn commented Dec 3, 2020

@hailwood great!

sorry forgot, please add a changelog (also next time: please don't delete the PR template, it's there for a reason to be reminded of such things, thank you!)

Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you 🤗

@barryvdh LGTM for merge!

@hailwood
Copy link
Contributor Author

hailwood commented Dec 3, 2020

Thanks @mfn
I normally keep the PR template, I think grammarly wiped it out sorry!

What would you put in the Changelog?
Thinking under Fixed

  • Casts with a return type of static or $this now resolve to an instance of the cast #1103 / riesjart

@mfn
Copy link
Collaborator

mfn commented Dec 3, 2020

@hailwood yeah, sounds good to me!

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