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

Add more helpful error messages on bad or insufficient morphTo data #1824

Merged
merged 3 commits into from Apr 24, 2018

Conversation

Projects
1 participant
@ricardograca
Copy link
Member

ricardograca commented Apr 23, 2018

  • Previous PRs: #656

Introduction

This adds more descriptive error messages when eager loading morphTo relations with bad or insufficient data. Also fixes an error about destructuring assignment when the target polymorphic couldn't be found, that prevented the actual expected error from being thrown.

Motivation

The current error is only thrown when the defined morph type can't be found in any of the possible target types, but the message is a bit ambiguous because it only says that the target model can't be found.

Additionally the changes proposed in the previous PR would introduce other potential issues. It's also not very clear that having a different behavior for a polymorphic relation when calling .related() is a bad thing, as argued in that PR, since polymorphic relations are a bit different from the others.

That PR also didn't include tests and its author was not responding.

Proposed solution

This adds an earlier check to see if the type attribute is set at all on the model and throws an error if it isn't. This is done before even attempting to find the target model from the defined target types.

The error message in case a valid target is not found is also reworded to make it clearer.

This is a breaking change since the existing error message is different and there is a new error being thrown.

Closes #656.

Current PR Issues

There is no check to see if the target id is set on the model since NULL can be a valid value, although it probably isn't. This check can easily be added in the future if needed.

@ricardograca ricardograca added this to To Do in Version 0.14.0 via automation Apr 23, 2018

@ricardograca ricardograca moved this from To Do to In progress in Version 0.14.0 Apr 23, 2018

@ricardograca ricardograca merged commit 1b3da40 into master Apr 24, 2018

1 of 2 checks passed

continuous-integration/travis-ci/push The Travis CI build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Version 0.14.0 automation moved this from In progress to Done Apr 24, 2018

@ricardograca ricardograca deleted the rg-null-morphTo branch Apr 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment