Fix relation name infering when MorphOne/MorphMany

[pxpm]

When using fq model namespaces for relationships, the MorphOne and MorphMany need to parse the FQN into a usable string when no key is provided.

This PR makes sure that if FQN is provided we will infer the name of the relationship from the namespace and don't use the namespace as the name.

Before:

After:

Thanks for your time and patience @jasonmccreary !

[jasonmccreary]

@pxpm, can you add a test to prove this change? Or comment with a fixture that would demonstrate it and I can add it.

[pxpm]

@jasonmccreary I'v just pushed the tests for this.

Please not that \fqn models have an opiniated naming as you can see here:

morphOne: \Other\Package\Order:stars, order:stars

// new syntax
public function order()
    {
        return $this->morphOne(\Other\Package\Order::class, 'starable');
    }

// old syntax
public function star()
    {
        return $this->morphOne(\App\Order::class, 'starable');
    }

When using the fqn to define the model, the relation name is always infered from the related model, and not from the relation_string as it happens in the "old" way.

blueprint/src/Generators/ModelGenerator.php

Line 205 in 17df0e3

$method_name = $is_model_fqn ? Str::afterLast($foreign_reference, '\\') : Str::beforeLast($column_name, '_id');

It's not a BC because it only applies to this new syntax, but my question here is: should I revert and use the same way of generating the method name, or do you think it makes more sense to generate it always from the model and probably in a next major version change the old syntax to generate too from model ?

Thanks,
Pedro

[jasonmccreary]

@pxpm, what is the "new" and "old" syntax? Can you provide examples of each. I've honestly never used Blueprint to generate polymorphic models.

[pxpm]

Sorry, what I meant by new syntax, is when using the \Full\Model\Namespace vs 'some_model'.

As you can see in the line I pointed you out in ModelGenerator.php, if \Fqn is provided the name is get from $foreign_reference, and previously it was got from $column_name. The difference that this produces is in the example I provided you.

This is not specifically to the fix provided in this PR, but in general for \FQN syntax. I am not aware if that produces undesired consequences in any other places, in doubt we can just revert that back for the same naming conventions when not using \FQN to define the relationship.

I am not sure if this is a feature but in BelongsTo, if you provide a second parameter you change the relation name:

belongsTo: slash:something_table_name, \custom:custom_table_name

public function somethingTableName()
    {
        return $this->belongsTo(\App\Slash::class);
    }

    public function custom()
    {
        return $this->belongsTo(\custom::class);
    }

If you provide both relation keys :

belongsTo: slash.some_key:other_key, \custom.some_key:other_key

    public function slash()
    {
        return $this->belongsTo(\App\Slash::class, 'other_key', 'some_key');
    }

    public function custom()
    {
        return $this->belongsTo(\custom::class, 'other_key', 'some_key');
    }

If changing the relation name is a feature, then the \FQN has a behaviour change, we currently can't change the relation name when using \FQN syntax, it's always infered from the relation model. It's not BC because it's contained into the new syntax.

Let me know,
Pedro

[jasonmccreary]

I guess I don't understand the colon syntax… Did this exist before?

morphOne: \Other\Package\Order:stars, order:stars

[jasonmccreary]

@pxpm, Ok I think this creates more symmetry between the two syntaxes.

One last question, seems the example draft file now generates two methods of the same name. I'd like to fix this only in the draft file. No need to have code check this. I consider it a user error. But I want the draft file to be clear as to the intent of the test.

[pxpm]

@jasonmccreary as far as I know, colon syntax is supported for multiple relationship definition, I saw it in Docs https://blueprint.laravelshift.com/docs/model-relationships/

I agree that now generating two methods with the same name seems unreasonable , should I remove the "old syntax" definitions and leave only with \FQN examples ?

Thanks in advance,
Pedro

[jasonmccreary]

Yes, or, ideally, create two separate test fixtures to demonstrate the newly added code.

[pxpm]

Hello @jasonmccreary

Thanks for the reply and the effort providing support.

I'v just updated the test to reflect this specific functionality, the tests without \FQN notation already exists.

Thanks,
Pedro