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

[8.x] Modify mergeCasts to return $this #35683

Merged
merged 1 commit into from
Dec 21, 2020
Merged

[8.x] Modify mergeCasts to return $this #35683

merged 1 commit into from
Dec 21, 2020

Conversation

finagin
Copy link
Contributor

@finagin finagin commented Dec 21, 2020

RP #35677 for 8.x branch

This change helpful to use the method in pipeline like this:

    public function initializeUsesUuid()
    {
        $this
            ->setKeyName('uuid')
            ->setKeyType('string')
            ->setIncrementing(false)
            ->mergeCasts([$this->getKeyName() => $this->getKeyType()])
            ->mergeGuarded([$this->getKeyName()]);
    }

P.S.

@GrahamCampbell this isn't a breaking change
#35676 (comment)

@finagin finagin changed the title [8.x] Modify mergeCasts to return `` [8.x] Modify mergeCasts to return $this Dec 21, 2020
@GrahamCampbell
Copy link
Member

Changing a void method to a non-void method in a non-final class is a breaking change. Anyone extending this class now fails to return the correct thing.

@driesvints
Copy link
Member

@GrahamCampbell there's no type annotation on the method. This breaks nothing in real code.

@dinhquochan
Copy link
Contributor

Yes, i agree @driesvints

@taylorotwell taylorotwell merged commit b0c5fbf into laravel:8.x Dec 21, 2020
@afraca
Copy link
Contributor

afraca commented Dec 21, 2020

If you have CI which does static analysis and fails hard (cannot deploy) when your model differs from base model I would call it breakage. I know this is far fetched, just saying things could break from some people.
https://xkcd.com/1172/

@driesvints
Copy link
Member

@afraca no actual code is broken. We don't optimize for static analysis tools.

@finagin finagin deleted the patch-1 branch December 23, 2020 07:38
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.

6 participants