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

feat: support SoftDeletes methods on relations #692

Merged
merged 6 commits into from
Oct 21, 2020

Conversation

jdrieghe
Copy link
Contributor

@jdrieghe jdrieghe commented Oct 20, 2020

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

Issue: #690

Changes

This introduces additional test cases to prove there is a regression issue that was introduced in version 0.6.6.
Methods from the SoftDeletes trait are not recognized on Relations even though this is supported through ForwardsCalls in Laravel.


Note that this PR does not yet introduce a fix but aims to clarify the issue defined in #690. I tried coming up with a solution but couldn't find it yet. Any pointers as to how to approach this would be welcome.

@canvural
Copy link
Collaborator

Hi,

Thanks for the failing test case.

I tried to explain a little bit what has to be done here: #690 (comment)

Let me know if you have any more questions.

…es trait and the methodName is one of the SoftDeletes methods (withTrashed, withoutTrashed or onlyTrashed)
@jdrieghe
Copy link
Contributor Author

@canvural I couldn't figure out how to return the proper MethodReflection for the SoftDeletes methods as they are added through a macro. I introduced the formerly used DummyMethodReflection for now to fix the BC break introduced in 0.6.6.

If you have a good solution on how to resolve the correct MethodReflection, I'm all ears. Thanks for your insights so far.

@jdrieghe jdrieghe changed the title Add failing cases for SoftDeletes methods on relations Support SoftDeletes methods on relations again Oct 20, 2020
@canvural
Copy link
Collaborator

canvural commented Oct 20, 2020

Hi,

Using DummyMethodReflection is not a good solution. We would lose the type information after the call.

As I wrote in #690 (comment) we should use the EloquentBuilderMethodReflection as the method reflection.

So in hasMethod method in RelationForwardsCallsExtension class, if $returnMethodReflection is null we need to check the method in model instead. $relatedModel->hasMethod($methodName)->yes() like this, and return the result.

Then in getMethod method again we can check if $returnMethodReflection is null we can return the reflection for the method with something like:

$originalMethodReflection = $relatedModelReflection->getMethod($methodName, new OutOfClassScope());

return new EloquentBuilderMethodReflection(
    $methodName, $classReflection, $originalMethodReflection,
    ParametersAcceptorSelector::selectSingle($originalMethodReflection->getVariants())->getParameters(),
    new GenericObjectType($classReflection->getName(), [$relatedModel]),
    false
);

I can push this fix this if you want 😊

We had to pull the logic out of the BuilderHelper and move it back in the RelationForwardsCallsExtension though.
@jdrieghe
Copy link
Contributor Author

Thanks @canvural. It's working now with the correct MethodReflection. The biggest issue I had was figuring out what scope to use, so your example with the OutOfClassScope was very helpful 👏

@canvural
Copy link
Collaborator

Perfect, thank you! Will merge when the build passes.

@canvural canvural changed the title Support SoftDeletes methods on relations again feat: support SoftDeletes methods on relations Oct 21, 2020
@canvural canvural merged commit 72733fe into larastan:master Oct 21, 2020
@szepeviktor
Copy link
Collaborator

szepeviktor commented Oct 21, 2020

I was waiting for this as v0.6.6 was producing SoftDelete-related errors.
Thank you!

@jdrieghe
Copy link
Contributor Author

@szepeviktor I encountered the issue on one of our internal projects, so it was mostly out of a personal need. Happy to have helped others though.

@canvural
Copy link
Collaborator

Released: https://github.com/nunomaduro/larastan/releases/tag/v0.6.7

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.

withTrashed() calls on BelongsTo are no longer allowed since version 0.6.6
3 participants