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.6] Make it easier to extend relationship classes #22382

Closed

Conversation

fico7489
Copy link

@fico7489 fico7489 commented Dec 10, 2017

what is new in this PR

  • with this PR we can easily change relationship classes

  • "imports", "classes array" and "method in a class" now have the same order :

hasOne
morphOne
belongsTo
morphTo
hasMany
hasManyThrough
morphMany
belongsToMany
morphToMany
  • currently if you want extend relation it looks like this :

https://github.com/fico7489/laravel-pivot/blob/master/src/Traits/ExtendBelongsToManyTrait.php

trait ExtendBelongsToManyTrait
{
    /**
     * Define a many-to-many relationship.
     *
     * @param  string  $related
     * @param  string  $table
     * @param  string  $foreignPivotKey
     * @param  string  $relatedPivotKey
     * @param  string  $parentKey
     * @param  string  $relatedKey
     * @param  string  $relation
     * @return \Illuminate\Database\Eloquent\Relations\BelongsToMany
     */
    public function belongsToMany($related, $table = null, $foreignPivotKey = null, $relatedPivotKey = null,
                                  $parentKey = null, $relatedKey = null, $relation = null)
    {
        // If no relationship name was passed, we will pull backtraces to get the
        // name of the calling function. We will use that function name as the
        // title of this relation since that is a great convention to apply.
        if (is_null($relation)) {
            $relation = $this->guessBelongsToManyRelation();
        }

        // First, we'll need to determine the foreign key and "other key" for the
        // relationship. Once we have determined the keys we'll make the query
        // instances as well as the relationship instances we need for this.
        $instance = $this->newRelatedInstance($related);

        $foreignPivotKey = $foreignPivotKey ?: $this->getForeignKey();

        $relatedPivotKey = $relatedPivotKey ?: $instance->getForeignKey();

        // If no table name was provided, we can guess it by concatenating the two
        // models using underscores in alphabetical order. The two model names
        // are transformed to snake case from their default CamelCase also.
        if (is_null($table)) {
            $table = $this->joiningTable($related);
        }

        return new BelongsToManyCustom(
            $instance->newQuery(), $this, $table, $foreignPivotKey,
            $relatedPivotKey, $parentKey ?: $this->getKeyName(),
            $relatedKey ?: $instance->getKeyName(), $relation
        );
    }
}

with my change It will be just

trait ExtendBelongsToManyTrait
{
    public static function bootExtendBelongsToManyTrait()
    {
        self::$relationClasses['belongsToMany'] = BelongsToManyCustom::class;
    }
}

and this is really awkward, especially if you have a different branch for each laravel versions because these relationship methods are different for each laravel version. Also, I see that many people want to extend relationship classes in packages and this PR will help them with that.

Solution is not fancy with interfaces and patterns but is better than the current.

@fico7489 fico7489 changed the title add array with relationship classes, all relationship methods use cla… [5.6] Make it easier to extend relationship classes Dec 10, 2017
@deleugpn
Copy link
Contributor

I like the concept, but I would rather have it on a new{Relation}() method that you can override. Similar to this https://github.com/laravel/framework/blob/5.5/src/Illuminate/View/Factory.php#L246 or this https://github.com/laravel/dusk/blob/2.0/src/TestCase.php#L135. It's easy to override the method and you don't have to instantiate a variable.

use Illuminate\Database\Eloquent\Relations\BelongsToMany;
use Illuminate\Database\Eloquent\Relations\HasManyThrough;
use Illuminate\Database\Eloquent\Relations\MorphToMany;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Imports should be sorted by length. Helps review the PR.

@fico7489
Copy link
Author

fico7489 commented Dec 11, 2017

@deleugpn I am not sure what are you mean.
1.I can't create new relation instance inside new newRelation method because each relation has different arguments and the completely different way how the instance is created.

But I only can do something like this :

protected static $relationClasses = [
    HasOne::class => HasOne::class,
    MorphOne::class => MorphOne::class,
    BelongsTo::class => BelongsTo::class,
    BelongsTo::class => MorphTo::class,
    HasMany::class => HasMany::class,
    HasManyThrough::class => HasManyThrough::class,
    MorphMany::class => MorphMany::class,
    BelongsToMany::class => BelongsToMany::class,
    MorphToMany::class => MorphToMany::class,
];

protected static function relationClass($relation){
    return self::$relationClasses[$relation];
}

public function hasOne($related, $foreignKey = null, $localKey = null)
{
    $instance = $this->newRelatedInstance($related);

    $foreignKey = $foreignKey ?: $this->getForeignKey();

    $localKey = $localKey ?: $this->getKeyName();

    $relationClass = static::relationClass(HasOne::class);

    return new $relationClass($instance->newQuery(), $this, $instance->getTable().'.'.$foreignKey, $localKey);
}

is this what you want ?

@deleugpn
Copy link
Contributor

Instead of return new $var(...) use return $this->newHasOne(...). Methods over magic variables.

@fico7489
Copy link
Author

@deleugpn
1.you have in mind one method for each relation :

newHasOne, newBelongsTo ...

or

2.something like :

public function __call($method, $parameters)
{
    $mapping = [
        'newHasOne'   => HasOne::class,
        'newMorphOne' => MorphOne::class,
        //...
    ];
    
    if(inArray($method, $mapping)){
        return new $mapping[$method]($parameters);
    }

    return parent::__call($method, $parameters);
}

@deleugpn
Copy link
Contributor

One for each.

@fico7489
Copy link
Author

fico7489 commented Dec 11, 2017

@deleugpn

If you mean like this :

1 .

protected function newBelongsToMany()
{
    return new BelongsToManyCustom(func_get_args());
}

then ok, but if you mean like this:

2 .

protected function newBelongsToMany($param, $param2, ...)
{
    return new BelongsToManyCustom($param, $param2);
}

is not ok for me because laravel have different argument count and types of relations between versions, and then I anyway must change newBelongsToMany() function for each laravel version what is not the goal. My idea is just to tell laravel which class should use for relation instance not define how the instance is created. I am a little bit skeptical because I can't find the example of the first way in laravel code, but I can find the second one...

What is the procedure now I should create new PR or I can commit to this one?

@fico7489
Copy link
Author

@deleugpn any news on this?

another example:

in this package, I extended "BelongsTo" and "HasOne" relations
https://github.com/fico7489/laravel-eloquent-join/blob/master/src/Traits/ExtendRelationsTrait.php

with current laravel core code, I have to have the separate branch for each laravel version (5.2 5.3 .5.4 5.5) with my PR I could have only one branch for all laravel versions.

It gets more complicated if I want to use more packages that extend the same relation, but with this PR, it could be much simpler, elegant and better.

@taylorotwell
Copy link
Member

If the arguments change between versions you are going to have to update your code anyways? Right?

@fico7489
Copy link
Author

fico7489 commented Dec 18, 2017

@taylorotwell If I could just define which class laravel should use then no if I have to define how this class should be created then yes.

'hasOne' => HasOne::class

vs

protected function newHasOne($param, $param2, ...)
{
    return new HasOne($param, $param2);
}

@deleugpn
Copy link
Contributor

I agree with Taylor. Both solution are exactly the same. The difference is that one uses an array and instantiate a magic variable and the other offers a method for you to override.

@taylorotwell
Copy link
Member

Going to hold off on this for now. I'm open to seeing the method based approach to see if I like it a bit better.

@deleugpn
Copy link
Contributor

Will try to pick this up after the holidays.

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.

None yet

3 participants