Fix relation types #1285
Fix relation types #1285
Conversation
|
|
|
All tests have passed :) We use FQCN in docblocks. |
Why the warning sign? Are you afraid of theoretical computer scientists? 😲 (BTW: You guessed correctly, I am indeed a theoretical computer scientist.)
I haven't expected less. I have run them locally first. 😏
OK. I will change that, but not today. It is getting late. I also still have to update the |
|
Hi, Thank you for this! While it makes sense to have the declaring class also in the generic types, I do not really see a use case for that. Even in the stubs you only used Can you give me a small code snippet where |
|
All my heart goes out to @nagmat84 |
First of all it fixes this bug: #1284 The bug is caused by the wrong assumption (and in consequence the wrong annotations) that the declaring model is the same as the related model.
That is simply not true. Let alone
It becomes useful in the very moment when you are working an real projects which also define their own relations because the default relations don't suffice. In that case one has to extend If you need examples, just have a look here Lychee: /app/Relations. At the moment we have to write hundreds of |
nagmat84
force-pushed
the
fix-relation-types-v2
branch
from
7898dd5
to
066eb25
Compare
|
I updated all PHPDoc comments to contain FQCN as requested by this comment. I also added a test case to illustrate why three times. Obviously, the methods Maybe, the problem occurs, because the template parameter is called |
nagmat84
force-pushed
the
fix-relation-types-v2
branch
from
372c0fb
to
f660f26
Compare
|
I believe I fixed the problem even though I have not fully understood what I did. In order to solve the "one-must-not-rename template-parameter-names" I stepped through the source code, watched out for the literal string As a second change I had to remove a condition in Obviously, removing the condition is required if one implements custom relations which shall work for all type of models and not only specific models. However, I don't know why the condition has been there in the first place. All tests are still running, hence the condition seems not to have had any impact previously and I assume it is OK to remove it. Unfortunately, the code is missing comments explaining why the original author added this or that condition. Someone with more knowledge about Larastan needs to look into that change thoroughly. I hope the PR is now fine to be merged. |
|
@canvural I think we should release it and see what custom relation developers have to say. |
|
I wonder how many there are. Obviously, I am one and I directly crashed into a lot of problems which led to this PR. So either the rest does not care and uses a lot of |
I support one of those developers: having a big |
|
What is the matter of state? When will this PR be merged? What can I do to push things forward? |
|
I'm not allowed to answer as I'm also sitting in my lab 🧪 but Larastan must be lazy developer compatible. p.s. @nagmat84 I would merge it right away |
|
Who is allowed to answer and has the authority to push things forward? @szepeviktor Thanks for your support. |
|
I'm currently really busy with work, and cannot review it now. I'll do it as soon as I can find some time. |
|
Is it possible to give a rough estimate when this PR will be reviewed or get merged? |
In two weeks. |
|
@nagmat84 You can see how we work on the rhythm of commits in master. |
|
There have been two two-weeks cycles since the last comment. What's the progress on this PR? |
Hi, We do not have any cycles of development here. 2 weeks was just an estimate of @szepeviktor I think there. Anyway, I apologize for the lack of communication here. Past few months were crazy busy for me. Work wise and personal issues. Only last week I found some time to get through some issues and pull requests. Made a new release with them. And still there are some issues/prs to review left. This is kinda a large PR, so first I spent time on smaller ones. I'll try to get to this as soon as possible though. Alternatively you can always motivate me through the sponsors: https://github.com/sponsors/canvural 😊 Thank you for understanding. |
|
I'll like to pose my usual question. ;-) |
this is not true, any users who have which forces the addition of such a phpdoc |
canvural
force-pushed
the
master
branch
2 times, most recently
from
6e390bd
to
325d2f3
Compare
|
We've closed the |
Resolves #1284
Changes
This PR
Relation-related classes andTRelatedModel:TDeclaringModelTResultTIntermediateModel(only in case of the...Through...-relations)Therewith it also resolves issue #1284.
Rationale
A relation links two models with each other which are not necessary identical. The
TRelatedModelis the type of model to which the relaton points to. TheTDeclaringModelis the type of model which declares the relation (i.e. the "base" of the arrow in the picture).Note that the roles
TRelatedModelandTDeclaringModelare independent of the definition of the foreign key on SQL layer. In the example above, onlyPosthas an attributetopic_idto realize a (1:*)-relation. But the relationshasManyandbelongsTolive on the "object space" and each points from its declaring model to the related model.In its most general form a
Relationhas a third template typeTResultwhich is not necessarily a model. TypicallyTResultis either identical toTRelatedModelor aCollection<int, TRelatedModel>. This is true for theHas...OneandHas...Many-relations. The latter also shows whyTResultmust not be restricted to a sub-type ofModel.However, for custom relations which extend
Relationthis must not be true. For example, instead of an Eloquent collection a custom relation could use another type of container, like aFixedSizeArray<TRelatedModel>which provides higher efficiency if one has a (1:n)-relation for a fixed n. (See #1275 (comment) for a real-world example.) However,TResultcould also be something like\DateTime. In the example above, a topic could have two relationsHasDateOfLeastRecentPostandHasDateOfMostRecentPostwhich resolve to the date/time of the least/most recent post. In other words, the result of a relation is a value which depends on the related model(s), but is not necessarily the related model(s).A Remark on Notation
I chose the term
TDeclaringModeloverTParentModelintentionally, although Laravel calls this model the "parent model" of the relation in most cases and the corresponding variable is called$parent. Seehttps://github.com/laravel/framework/blob/9.x/src/Illuminate/Database/Eloquent/Relations/Relation.php#L30-L35
However, it does not do so consistently. In
BelongsTothe names are not used according to their usage in the code but are based on the use case. The declaring model is named the "child model" and the "parent" becomes the related model. Seehttps://github.com/laravel/framework/blob/9.x/src/Illuminate/Database/Eloquent/Relations/BelongsTo.php#L62-L65
Even worse
BelongsTodoes not apply this change of names consistently, i.e. the related model and the declaring model are both called the parent model. In order to avoid strange constructs likeTParentModel $child, I decided to useTDeclaringModel. Moreover, the term "declaring" class has already been used inside the Larastan sources.This gives the following table
Relation,HasMany,HasOne, ...,BelongsToManyBelongsTo(only)TRelatedModel$related$relatedTDeclaringModel$parent$childBreaking changes
The
Relation-related classes now require up to four template parameters.There should be no breaking change for ordinary users which only use the provided relations and create relations via the methods
hasMany,hasOne, ..., etc. provided by\Illuminate\Database\Eloquent\Concerns\HasRelationships. The classesModelRelationsDynamicMethodReturnTypeExtensionandRelationDynamicMethodReturnTypeExtensionhave been adopted to bound all template parameters accordingly.A breaking change may occur for users which implement their own relation classes and extend from
Relationor its sub-classes. They need to bind the newly added template parameters. But I assume this group to be rather small. Otherwise someone else would already have recognized this bug earlier.