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

[5.1] [5.2] Optional morphTo relationship fatal error when not set #11169

Closed
AdenFraser opened this issue Dec 3, 2015 · 11 comments
Closed

[5.1] [5.2] Optional morphTo relationship fatal error when not set #11169

AdenFraser opened this issue Dec 3, 2015 · 11 comments
Labels

Comments

@AdenFraser
Copy link
Contributor

i've just encountered an issue where when a morphTo relation is not defined for a model instance but is called, lets say with if ($model->morphRelation()) { then a fatal error occurs:
Fatal error: Class '' not found in /vagrant/vendor/laravel/framework/src/Illuminate/Database/Eloquent/Model.php on line 875

This seems like unexpected behaviour since if the relationship is optional, the expected response would be null or false (I'm assuming null) rather than an attempt to return a class based on the empty morph_type column.

This has been discussed as an issue briefly before, but didn't appear to go anywhere:
#1450

I was wondering if this is expected behaviour, a bug or perhaps an enhancement could be made to allow for optional morphTo relations? If that's the case I'd look into it and create a PR with the outcome.

It just seems odd that every other relation either usually returns null (in the case of single relations not existing) or an empty collection (in the case of multi relations not being found) but with morphTo it does not.

Thoughts?

@GrahamCampbell
Copy link
Member

Hmm. Thanks for reporting. This does look like a bug. Are you able to send a PR?

@AdenFraser
Copy link
Contributor Author

Sure, I'll take a look into it tomorrow if I get a chance :)

@GrahamCampbell GrahamCampbell changed the title Optional morphTo relationship fatal error when not set. [5.1] [5.2] Optional morphTo relationship fatal error when not set Dec 20, 2015
@RobertYearwood
Copy link

Just weighing in as I hit this same issue.

This is technically not a bug, but a side-effect of the value for the morph_type field being an empty string ('') instead of a proper null.

The fix that we implemented involved copying the morphTo function from Illuminate\Database\Eloquent\Model.php to our base model and expanding the is_null check to also check for an empty string.

Specifically this:

if (is_null($class = $this->$type)) {

Turned into:

if (is_null($class = $this->$type) || $class === '') {

After that, it worked as expected for us.

As a side note: I can't think of a valid instance where an empty string shouldn't be considered null for this check, as it's impossible to instantiate an empty string to a class. At the very least, this should throw an application exception of some variety instead of letting it pass through to an easily preventable fatal error.

@vjoao
Copy link

vjoao commented Mar 16, 2016

Any fix on this?

@garygreen
Copy link
Contributor

Bump. I've having this same problem as well, it's even a problem on 4.2 upwards. Every other relationship allows for nullable but why not morphs?

@vaites
Copy link
Contributor

vaites commented Apr 28, 2016

You can use an accessor that check if has a relation:

public function getNotificableAttribute()
{
    if(!is_null($this->notificable_id))
    {
        return $this->relations['notificable'] = $this->notificable()->getResults();
    }
}

public function notificable()
{
    return $this->morphTo();
}

@AdenFraser
Copy link
Contributor Author

@vaites This is similar to a solution that I've been using too.

Never got around to putting together a PR... Guess i really should!

@vaites
Copy link
Contributor

vaites commented Apr 28, 2016

@AdenFraser I think there's no need to modify Laravel behaviour, there are only a few cases and is solved with only 5 lines of code. Maybe a better documentation on Eloquent relations suggesting an accessor...

@AdenFraser
Copy link
Contributor Author

@vaites I disagree. While what you propose would be simpler, this is inconsistent behaviour with all other relation functionality.

@RobertYearwood
Copy link

It's a single line modification to resolve it and make it consistent with no breaking changes.

@AdenFraser
Copy link
Contributor Author

@RobertYearwood Exactly. I'm putting in a PR now.

AdenFraser pushed a commit to AdenFraser/framework that referenced this issue Apr 28, 2016
AdenFraser pushed a commit to AdenFraser/framework that referenced this issue Apr 28, 2016
AdenFraser pushed a commit to AdenFraser/framework that referenced this issue Apr 28, 2016
AdenFraser pushed a commit to AdenFraser/framework that referenced this issue Apr 28, 2016
AdenFraser pushed a commit to AdenFraser/framework that referenced this issue Apr 28, 2016
AdenFraser pushed a commit to AdenFraser/framework that referenced this issue Apr 29, 2016
AdenFraser pushed a commit to AdenFraser/framework that referenced this issue Apr 29, 2016
taylorotwell pushed a commit that referenced this issue Apr 29, 2016
zither added a commit to zither/framework that referenced this issue Apr 30, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants