[RTM] Always fix the domain and language when generating URLs

[leofeyer]

This PR fixed #8204 and all duplicate tickets for good (hopefully).

[aschempp]

Why do we need still the $strForceLang?

[aschempp]

Also, can we add support for $arrRow being a PageModel?

[leofeyer]

Thought about it, too. Maybe not in the bugfix release.

[aschempp]

What's the difference? If we always force domain, we always force the language…

[leofeyer]

Why do we need still the $strForceLang?

We do not need it, we just keep supporting it for BC. Not sure what people have used it for …

[Toflar]

Not sure what people have used it for …

To force the language, like the parameter says? If you don't have any front end context, how would you generate it correctly? :-)

BTW: I think I have a better approach for your code performance wise which will be fully backwards compatible:

• Check if tl_page.language has changed in save_callback and if so, update all child records with it.
• Modify the PageModel and introduce a getLanguage() method:

public function getLanguage()
{
    // Ensure root page language is inherited if not set by save_callback (BC)
    if ('' === $this->language) {
        $this->loadDetails();
    }

    return $this->language;
}

• Support passing the \PageModel to \Controller::generateFrontendUrl() then instead of using $arrRow['language'] use \PageModel::getLanguage().

Why in 3.5: Well, we have to change the handling because it's buggy at the moment but if we already change it then we should make it right :-)

[leofeyer]

To force the language, like the parameter says? If you don't have any front end context, how would you generate it correctly? :-)

No front end context is required. It will set the language and domain based on $page->rootLanguage and $page->domain, which are both set in PageModel::loadDetails().

I think I have a better approach for your code performance wise which will be fully backwards compatible

How is your code performing better? We'd still have to bubble up to the root page to check the domain, wouldn't we?

[leofeyer]

Not to forget $page->trail, which also required to load the page details.

[leofeyer]

The PR is ready to merge now. Please review one last time.

[aschempp]

If I remember correctly, we intentionally do not use models here because of performance and memory …

[leofeyer]

I don't remember anything alike. Has there been a ticket?

[Toflar]

I remember something like this as well. Cannot find it though but it sure is an issue with a lot of pages. Please do not change this!

[leofeyer]

I have to change it to fix the initial problem.

[Toflar]

This will fail miserably, I tell you. This is a no go, I'm sorry.

[leofeyer]

You are telling me this based on something you think you remember, which however is nowhere documented?

[aschempp]

#7928 (comment)

[leofeyer]

I have investigated a bit. Here are some findings of where not to use models:

• https://github.com/contao/core/blob/master/system/modules/core/forms/Form.php#L525-L526
• https://github.com/contao/core/blob/master/system/modules/core/classes/Theme.php#L560-L561

Rebuilding the search index is not one of them. Also, I have debugged rebuilding the search index with and without models:

With models

Without models

Using models requires 10% more memory, however, it also decreases the number of database queries and the number of rows which need to be transferred.

@Toflar What exactly are your concerns?

[Toflar]

Try this on an installation with 3000 pages that are split over 12 domains times 4 languages (= 48 root pages). I think it's going to fail 😄

[leofeyer]

I found the ticket about what you think to remember: #7928

We have also discussed that we need to use the database class again, but then we found a bug in the Model class, which turned out to cause the problems.

After fixing the bug, the memory issues were gone. So we can safely merge this PR now.

[aschempp]

After fixing the bug, the memory issues were gone.

That's just not true. The last comment was "please test", so we don't know if anything changes. The major difference (in my opinion) is that the models will be kept in the registry indefinitely, whereas the database result will be removed from memory when not used anymore.

[leofeyer]

Of course they were gone. We have tested the change extensively, we have just not commented in the ticket. But it was only closed two weeks after finding the bug.

[Toflar]

I tried to do some tests to back my use case and ended up with a regular page model requiring more or less 20kb of RAM. So for 3000 pages it would require about 60 MB RAM which is fine then I think :-) If they are split over 48 root pages it will certainly need quite a lot more memory but it should still be fine. So I guess my concerns are refuted :-)

However, I still think, inheriting dns, language and whatever other columns are needed to generate a front end url, is the way to go. You can safely generate a front end url from one single database row without having to load the model and its root page and bubble up all the pages to load the details then. It would greatly improve performance, memory consumption and developer experience :-)

[aschempp]

However, I still think, inheriting dns, language and whatever other columns are needed to generate a front end url, is the way to go. You can safely generate a front end url from one single database row without having to load the model and its root page and bubble up all the pages to load the details then. It would greatly improve performance, memory consumption and developer experience :-)

That's certainly something we should think about, but certainly not in the 3.5.7 patch release ^^

[leofeyer]

However, I still think, inheriting dns, language and whatever other columns are needed to generate a front end url, is the way to go.

Yes, I'm totally with you on this one. However, that is a different issue.

The problem is that storing the fields in the database is pretty cumbersome and also error-prone, because you have to keep it in sync when the root page changes. There are some solutions for this problem already and backbone87 has implemented one in #6193 (his initial issue was #6830).

[leofeyer]

So now that we have settled this and that I have changed all you have suggested, can I merge the PR?

[Toflar]

Imho, yes.

[leofeyer]

Ah, no, I cannot. I have not solved this issue brought up by @aschempp :(

[leofeyer]

You could have added a flag to $arrOptions to disable the fePreview check?

I have thought about exactly this, too 😄

I have not asked you though, because I also think it is a new feature. But we should definitely implement this!

[leofeyer]

I have created a separate ticket: contao/core-bundle#452

[leofeyer]

Merged in c68d813.