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

Decoupled models from managers #260

Merged
merged 1 commit into from
Feb 16, 2021

Conversation

denisvanmorgan
Copy link

I stumbled upon this problem recently where I needed to override doctrine mappings with my custom mappings and models as well thus override some manager classes to use my models. While doing this I realized that manager interfaces are dependant on concrete classes rather than abstractions. This change should bring more flexibility and freedom when overriding default managers with custom models.

@MichaelKubovic
Copy link

Hey @spideyfusion could you please consider this merge? It is fully backward compatible and may solve others issue as well.

@MichaelKubovic
Copy link

Friendly ping. Is there anything blocking this PR?

@HypeMC HypeMC changed the base branch from v3.x to master February 16, 2021 17:51
@X-Coder264
Copy link
Collaborator

@MichaelKubovic Unfortunately this is a breaking change for people who already implemented the interfaces on which you've widened the type from the concrete class to the interface so we'll merge this into master.

Fatal error: Declaration of Y::bar(BazInterfaceImplementation $baz) must be compatible with FooInterface::bar(BazInterface $baz)

http://sandbox.onlinephpfunctions.com/code/e71ddc05989b9aac2654c19bdab5863adc9362ad

@HypeMC HypeMC merged commit 09462b1 into trikoder:master Feb 16, 2021
@HypeMC
Copy link
Contributor

HypeMC commented Feb 16, 2021

@denisvanmorgan Thank you for your contribution.

@denisvanmorgan
Copy link
Author

@HypeMC happy to contribute, thank you for your review

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.

4 participants