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

setMethods deprecation: combining addMethods+onlyMethods is not possible (Cannot use … on … mock because mocked methods were already configured.) #3858

Closed
kalessil opened this issue Sep 9, 2019 · 6 comments
Labels
type/bug Something is broken

Comments

@kalessil
Copy link

kalessil commented Sep 9, 2019

First, I like the split made with the new API =)

Still, setMethods, as I understood from the PR, has recommended replacement by addMethods AND onlyMethods. But implementation requires to use addMethods OR onlyMethods, what is seems to be imcomplete.

In practical field it breaks existing UTs, and gives no possibility to mimic original behavior.

IMO the new methods should be possible to combine as '->onlyMethods(['where', 'first'])->addMethods(['withNoStatusFlags'])'.

@kalessil kalessil added the type/bug Something is broken label Sep 9, 2019
@jarenal
Copy link

jarenal commented Sep 24, 2019

Same issue here, any news about this?

@ernstvanrijn
Copy link

Same issue here as well. I agree with kalessil's solution.

@bikalbasnet
Copy link
Contributor

With reference to this comment #3687 (comment) from @sebastianbergmann , I can see one point

Figure out whether addMethods() and onlyMethods() should be combinable (I don't think they should be)

I am not sure if this has been decided to not to be combined but I am hoping we could get a clear answer on this.

@DFoxinator
Copy link
Contributor

I thought it would be better to not have them be combinable at first, but now after using them in practice a lot, I do see the use of being able to combine them. The combined use is limited I think, but it's there.

The only times it has made sense for them to be combined in the codebase I work on (that has thousands of tests) is a few instances where the AWS SDK is used, which unfortunately abuses magic methods. But there might be other occasions like that so it might make sense to have the ability to combine.

@bikalbasnet
Copy link
Contributor

bikalbasnet commented Oct 28, 2019

I agree it totally makes sense to combine to me as well because of the magic methods. One of the ORM that I am using uses a lot of magic methods for getters and setters. I could directly call a function that does not exists

$this->getFirstName()

instead of creating a function getFirstName() function.

Sometime we might need to mock methods that exists and does not exists together. 😓

@sebastianbergmann
Copy link
Owner

Superseded by #3911.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug Something is broken
Projects
None yet
Development

No branches or pull requests

6 participants