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] Added morph map #9891

Merged
merged 2 commits into from Sep 3, 2015

Conversation

Projects
None yet
6 participants
@phroggyy
Copy link
Contributor

commented Aug 9, 2015

This is the same as #9839 but I couldn't reopen the pull request after force pushing

@phroggyy phroggyy referenced this pull request Aug 9, 2015

Closed

[5.1] Added morph map #9839

$class = get_class($this);
if (! empty($morphMap) && in_array($class, $morphMap)) {
return array_flip($morphMap)[$class];

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Aug 9, 2015

Member

Performance of this function is surely lower than if you were to use array_search.

This comment has been minimized.

Copy link
@phroggyy

phroggyy Aug 9, 2015

Author Contributor

Good point, changed it

@@ -273,6 +280,26 @@ public function wrap($value)
}
/**
* Set the morph map for polymorphic relations.
*
* @param array|null $map

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Aug 9, 2015

Member

missing space

* Set the morph map for polymorphic relations.
*
* @param array|null $map
* @param bool $merge

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Aug 9, 2015

Member

missing space

*/
public static function morphMap($map = null, $merge = true)
{
if (is_array($map)) {

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Aug 9, 2015

Member

You can reduce nesting by doing the reverse of this and early returning static::$morphMap.

This comment has been minimized.

Copy link
@phroggyy

phroggyy Aug 9, 2015

Author Contributor

@GrahamCampbell Would that imply getting rid of the is_array check? Or do you merely mean a return inside the if statement to avoid the else?

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Aug 9, 2015

Member

No, I'm saying do ! is_array() first.

$class = get_class($this);
if (! empty($morphMap)) {
return array_search($class, $morphMap);

This comment has been minimized.

Copy link
@GrahamCampbell

GrahamCampbell Aug 9, 2015

Member

pass a third param true to increase performance by forcing strict comparisons

@brunogaspar

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2015

I haven't looked into the tests, so not sure if the morphMap method is really well tested/covered, but why does the $map is not casted to array? It seems that an array is expected all the times, when provided, so it should be casted, no?

But either way, i believe the method can be tweaked to be something like:

public static function morphMap(array $map = null, $merge = true)
{
    if (! is_null($map)) {
        if ($merge) {
            $map = array_merge(static::$morphMap, $map);
        }

        static::$morphMap = $map;
    }

    return static::$morphMap;
}

This way there's less repetition with the return static::$morphMap; and i noticed that when you're merging you're not storing the merge, you just return, is that the intended behavior?

Sorry if i've missed something.

@phroggyy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2015

@brunogaspar: thanks for the input, and especially for pointing out that I'm not setting it there! Anyway, the reason I'm checking !is_array is so that I can just return the map instead of throwing an error if an invalid value is passed in. Whether this should be the intended functionality is all up for discussion. The way it currently works was based on

You can reduce nesting by doing the reverse of this and early returning static::$morphMap.

Also, not type casting but type hinting. I'm changing that now (although hinting array and providing null feels weird).

@JosephSilber

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2015

You can use a ternary:

public static function morphMap(array $map = null, $merge = true)
{
    if (is_array($map)) {
        static::$morphMap = $merge ? static::$morphMap + $map : $map;
    }

    return static::$morphMap;
}

The intent of this method now seems a lot clearer to me.

@phroggyy phroggyy force-pushed the phroggyy:5.1-feature/morph-map branch from ab374cb to 719f391 Aug 12, 2015

@phroggyy

This comment has been minimized.

Copy link
Contributor Author

commented Aug 12, 2015

Thanks for the suggestion @JosephSilber, updated the method!

@GrahamCampbell

This comment has been minimized.

Copy link
Member

commented Aug 12, 2015

This is looking much better now. :)

@tomschlick

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2015

Looks good to me! 👍

return $morphMap[$class];
}

return $class;

This comment has been minimized.

Copy link
@JosephSilber

JosephSilber Sep 3, 2015

Contributor

You can replace everything in this method with:

return array_get(Relation::morphMap(), $class, $class);

@taylorotwell taylorotwell merged commit 719f391 into laravel:5.1 Sep 3, 2015

2 checks passed

StyleCI The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@JosephSilber

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2015

👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.