Allow global page model access in fragments

[fritzmg]

Q	A
Fixed issues	-
Docs PR or issue	-

Currently fragment controllers for content elements and front end modules only receive the ID of the page model of the current page in their pageModel request attribute, whereas everywhere else the pageModel request attribute is actually the global page model object itself.

This means that fragments are not able to access the global page model object at all, if they do not want to use global $objPage or $GLOBALS['objPage'], since the global page object is detached from the model registry. Thus you couldn't implement something like ModuleNewsreader as a fragment, without resorting to accessing the global variable, since the module needs to be able to modify the current global page object.

As far as I understand it, the pageModel only needs to be an ID for esi rendered fragments, but not really for inline or forward rendered fragments. Therefore this PR would change this behaviour.

However - why does this distinction need to be made at all? Isn't the global page object missing anyway (and thus also the pageModel request attribute), when rendering an esi fragment?

[aschempp]

I'm not sure this is the correct approach. Having the fragments behave differently depending on the renderer isn't really good to me. Also, if your fragment's __invoke method has an argument for PageModel $pageModel, the object would automatically resolved through our controller resolver.

I agree its cumbersome not to have the page object. But I think we should have a way where the behavior is the same for all fragment types. The only question to me is whether the (non-esi) fragments must have access to the real global $objPage or if it could be a new instance?

[fritzmg]

Also, if your fragment's __invoke method has an argument for PageModel $pageModel, the object would automatically resolved through our controller resolver.

Huh, I didn't know that, may be we should document that 😁 . And this will be the global page object?

The only question to me is whether the (non-esi) fragments must have access to the real global $objPage or if it could be a new instance?

Well, if it's a new instance, then the module cannot change the page title, description etc., like the newsreader module would do for example.

[aschempp]

Also, if your fragment's __invoke method has an argument for PageModel $pageModel, the object would automatically resolved through our controller resolver.

Huh, I didn't know that, may be we should document that 😁 . And this will be the global page object?

If your function argument is a subclass if Model and a (numeric) request attribute with that name exists, our argument resolver will inject the model instance for you. This is how the abstract fragment controller gets $contentModel or $moduleModel injected.

Be aware that you can inject any request attribute automatically in your controller, your listener just converts the attribute.

The only question to me is whether the (non-esi) fragments must have access to the real global $objPage or if it could be a new instance?

Well, if it's a new instance, then the module cannot change the page title, description etc., like the newsreader module would do for example.

That's true. Changing the page title would obviously not work for ESI requests anyway. However, I'm not sure we should add support for that – specifically because doing that is actually kind of wrong: the page controller should (be able to) define the title, not through a fragment. But thats another story...

[fritzmg]

However, I'm not sure we should add support for that – specifically because doing that is actually kind of wrong: the page controller should (be able to) define the title, not through a fragment. But thats another story...

Yes, but that's what we have to do in the current state of Contao. And so currently in a fragment controller, you still need to use global $objPage in order to do that.

And even if you don't need to do that and only need information from the page object, it is still more efficient to access global $objPage in your fragment controller! Otherwise your fragment controller will cause an additional database query, if it loads the page model from the page model ID from the request attribute (since the global page object is detached from the model registry).

[m-vo]

To be honest, I found it strange ever since that the 'global page' model could be altered during the request. It's like starting an edit but never saving it (and therefore the need to detach it from the manager). Imo we should find a better abstraction for this and maybe use global $objPage until then.

[aschempp]

I tend to agree with @m-vo. Since we introduced page controllers in Contao 4.10, we should probably consider this "the solution" to modifying the page title. Only pages are supposed to tell about the page title, none of the fragments should take care of this.

[fritzmg]

Yes, but the issue isn't just about altering the global page object. It's also about simply accessing it. The current implementation leads to an additional, unnecessary database query. Surely it would make sense to prevent this?

And even if you implement a page controller in the future: the fragment still needs access to the model of the page.

[bytehead]

I've just stumbled over this, usingFrontend::getRootPageFromUrl() does not work in fragments, because the request attribute pageModel is an ID and not the page object.

Fun fact: in $request->attributes->get('_route_params') is the pageModel available as object...

[aschempp]

Fun fact: in $request->attributes->get('_route_params') is the pageModel available as object...

most likely because it has been copied from the original request.

--

Maybe we should add a service that provides the current page model instead of retrieving it from the request parameter? That service could then look into the request or $GLOBALS. Not sure what that service would look like though (and it obviously would only be available in 4.11 or later).

[fritzmg]

I still consider it a bug that the pageModel is only an ID in fragments ;)

[leofeyer]

See #2506