Register models only once if they are related

[Wusch]

If you have a model related to itself it will be registered twice because the constructor of the model will register it. And this will create an Exception in the registry.

[aschempp]

How can a model be related to itself? That sounds like a design error?

[Wusch]

We have categories for our courses and every category is followed by another one. Sometimes they just end and have no one following but some course categories repeat infinitely. That's why they are related to themself.

[aschempp]

So you mean a category is related to another category. But the same category is not related to itself (same ID).

[Wusch]

It can be possible that the same category is related to itself.
id => 38 followed_by => 38

[discordier]

Which will be an endless recursion IMO.

[Wusch]

No it will not because new relations are checked if they are in the registry. If not they will be created an added to the registry. But if they are already in the registry they will be merged and that's it.
The model than has a relation to itself.

The problem in the current version is that an exception is thrown because it's registered twice.

[aschempp]

It can be possible that the same category is related to itself.
id => 38 followed_by => 38

as I said, this looks like an error in your application design, because it would result in an infinite recursion.

[Toflar]

Which I guess is the reason to remove the register() call 😄 But I agree, it is not correct to remove that line there 😄

[Wusch]

But if you remove that line there the related model will still be registered in his constructor.

[aschempp]

No it will not, because there is no constructor argument so that step is skipped...

[Wusch]

$objRelated = new $strClass(); on line 166 will create a new class/model?

[aschempp]

Yes. But because there's no constructor argument, line 117 won't be true and the model will not be added to the registry in line 177.

[Wusch]

Oh, my mistake. I updated the file and added a ne if at the second register which checks if the model is already registered. Probably that should solve the problem.

[aschempp]

Now that sounds more reasonable :)
But I wonder why we would then need to build the model at all, why not check the ID in the registry first?

Also, I can't explain why this was not an issue before, because eager relations would always be loaded multiple times?

[leofeyer]

@contao/developers What is the status of this PR? Shouldn't it be merged into Contao 3.5.10?

[Toflar]

I think no. I think this is not the right solution and I'm not sure if there is any. Can you make this up for discussion?

[leofeyer]

Sure. But the current changes just make sure there is no registration attempt if the module has been registered before. That's reasonable IMHO.

[Toflar]

No, it's not. It's not the responsibility of the model that creates itself to know whether it should register itself to the registry or not. That's very weird.

[Wusch]

But it solves my problem and if you merge it i don't have to overwrite the file every time there is an update.

[Toflar]

I did not say the problem does not exist, I said I think the solution feels wrong.

[Wusch]

If you don't want to change the model you can change the registry class on line 180 and just move on and don't throw an error.

[Toflar]

That's not correct either. You cannot have two instances working for the same row, that's why I'm saying this needs to be discussed.

[leofeyer]

@Wusch How did you discover the error? And how do we reproduce it?

[Wusch]

We have templates for our courses where you have to select the following course. Some have no and some have another course following them. But one or two at the end of the line you can repeat infinitely. Which means that they are related to themselves.

[leofeyer]

As discussed in Mumble on April 21st, the following change fixes the issue:

diff --git a/system/modules/core/library/Contao/Model.php b/system/modules/core/library/Contao/Model.php
index 5ad3a48..7017407 100644
--- a/system/modules/core/library/Contao/Model.php
+++ b/system/modules/core/library/Contao/Model.php
@@ -138,6 +138,9 @@ abstract class Model

            $objRegistry = \Model\Registry::getInstance();

+           $this->setRow($arrData); // see #5439
+           $objRegistry->register($this);
+
            // Create the related models
            foreach ($arrRelated as $key=>$row)
            {
@@ -172,9 +175,6 @@ abstract class Model
                    $this->arrRelated[$key] = $objRelated;
                }
            }
-
-           $this->setRow($arrData); // see #5439
-           $objRegistry->register($this);
        }
    }

[leofeyer]

Fixed in 12fe550.