[RTM] Fragments as controllers

[Toflar]

So I would like to reopen this case here :-) Basically, Contao consists of 4 different elements:

• page types
• content elements
• frontend modules
• insert tags

All of these clearly are regular controllers. At the beginning I thought it would be a good idea just to register them as a regular route to Symfony and provide them some special attributes. But there are several issues with this approach:

Every bundle would need to provide its own routing.yml and this is cumbersome. Even if used with the managed edition it would be a nightmare for the developers to add a new content element. I mean like how do you name your route? How can you prevent conflicts? Is there a convention? What kind of attributes can I give my route? etc.

In my vision, it should be as simple as a service tag and implementing an interface that guides you to what you need to do. This PR does exactly that (I've implemented it for page types only for now, as this is WIP).

services.yml

services:
    my_bundle.whatever_i_like.controller.page_types.my_page_type:
        class: WhereverIWantToPlace\MyControllerForPageType
        public: false
        tags:
            - { name: contao.page_type }

MyControllerForPageType.php

<?php
class MyControllerForPageType implements PageTypeInterface {}

That's it. You're done. The interface will tell you what you need to do and the rest is done by Contao.

At the beginning I wanted to make this PR for page types only but it all works the same. It's just different interfaces:

• Page types do not need to be rendered in the back end so no need for it in the interface. Content elements and front end modules do.
• A page type will never be rendered as an esi fragment because it is the "master" for a given alias. However, the real route is still the contao_frontend_index so technically speaking, it is a fragment ;-) But content elements, front end modules and insert tags might be rendered as ESI or hinclude etc. So their interfaces could be extended by a getRenderStrategy() for instance.

ToDo's:

• Define if this is generally an approach that we all like
• Discuss whether we should add insert tags already, I'm not yet sure about its implementation so maybe we should add it later
• Implement content elements and front end module fragments (and, depending on the decision insert tags)
• Tests
• Lazy proxy instantiation

Thanks for the feedback!
// @contao/developers

[ausi]

Having content elements as controllers means that they get only a request object (with some special attributes) and return a response object, right? Is it then still possible for a content element to add some resources to the page like it is now with $GLOBALS['TL_CSS']?

Basically, Contao consists of 4 different elements: page types, content elements, frontend modules, insert tags

What is the difference between a content element and a frontend module? Should we make them the same thing?

Define if this is generally an approach that we all like

I think I like it in general, but I’m not sure about If they have to be controllers and receive HTTP requests and return HTTP responses. Would’t it be better if a content element receives its data and returns the rendered output and eventually some resources that should be added to the whole page (CSS, JS,...). To render a content element in a fragment it could get wrapped in a ContentElementController that internally renders the element and returns it as a HTTP response object.

[Toflar]

Is it then still possible for a content element to add some resources to the page like it is now with $GLOBALS['TL_CSS']?

I think we should not discuss this here because it's not related to that PR. And you know the answer ;-)

What is the difference between a content element and a frontend module? Should we make them the same thing?

They are the same thing which is why I built a general FragmentRegistry. However, they differ because we have them at different places in the back end :-)

I think I like it in general, but I’m not sure about If they have to be controllers and receive HTTP requests and return HTTP responses. Would’t it be better if a content element receives its data and returns the rendered output and eventually some resources that should be added to the whole page (CSS, JS,...). To render a content element in a fragment it could get wrapped in a ContentElementController that internally renders the element and returns it as a HTTP response object.

Honestly, a proper HTTP response is the only way to do it right. Otherwise you have to provide too much stuff. Like you wrote: resources, output. But there's more! For example caching (and thus all kinds of caching types, from e-tag to expiry, vary headers etc.).
The Response is exactly what we need, a container for it all.

[aschempp]

Generally I like the idea of the controllers with tags, this is what we thought about years ago. However, I would not use interfaces as it would only allow one "element" per controller. We'd better use a tag attribute for method as other events do.

[Toflar]

We'd better use a tag attribute for method as other events do.

A what?

[aschempp]

like this: { name: contao.page_type, method: someAction } We could also add more properties e.g. an id for the internal type field (drop down) and language reference.

[Toflar]

However, I would not use interfaces as it would only allow one "element" per controller. We'd better use a tag attribute for method as other events do.

I disagree because this is bad software design. It violates the SRP.
It maybe makes sense for an event listener because there are listeners that need to listen on two events to fulfill their purpose (= SRP). But it never makes sense to combine two page types, content elements or modules into the same class. That's nonsense.

To come back to @ausi's comment about resources. I thought about that and I found the solution for that by asking myself who's responsible for the resources and it's the page. So the page has to ask the elements for their resources. And with my approach it would be as simple as implementing a ProvidesResourcesInterface.

So for a content element:

class MyContentElement implements ContentElementInterface, ProvidesResourcesInterface
{
    public function getName();
    public function getGroup();
    public function renderAction(Request $request);
    public function renderBackendAction(Request $request);
    public function getResources();
}

And all of a sudden, multiple problems we have today are solved:

• We will be able to ask insert tags for their resources.
• Our alias content element that itself includes another one, can easily provide the resources of the other one by just calling getResources() again and passing it on.
• more?

You could never do that when combining multiple responsibilities into one class.

We could also add more properties e.g. an id for the internal type field (drop down) and language reference.

Yes, I would place it into my interfaces because e.g. content elements need a group, page types don't. See example above. The PR is not finished so I might find more methods that are needed ;-)

[aschempp]

Maybe you should also think about flat controllers as well. The listener class must not necessarily render the content element, but it would use injected services to do so. As an example, the download and downloads content elements are very similar, I would use the same controller and potential helper classes to render them.

[Toflar]

Same applies for controllers. They should always only render one action to follow the SRP.
Symfony sort of advocates having multiple actions per controller and this is wrong. It's better to only have one action per class because you only inject the dependencies needed by this very action.
Sure, there might be actions that are very similar where it's okay to share but it should be the exception, not the rule!
That's also why there are a lot of discussions going on in the Symfony environment regarding this topic and why libraries such as Kevin's ActionBundle are very popular.

Our content elements share nothing in 95% of all cases. That's why we should never render multiple elements in one class.
If download and downloads are almost the same (which again is the exception!), feel free to build an abstract parent class that implements all they share and you automatically have the class where your helpers should be placed :)

[discordier]

I agree on the SRP issue in general and look forward to how this PR evolves.

I have some questions on the resources idea outlined above though.
IMO this only applies to static resources and will not solve the problem of custom compiled assets (like we have in the body now). Therefore this still needs to be solved or we all agree that this code should then end up in the twig templates of the content elements.
Am I right on that one?

[Toflar]

Collecting would work as it does now. Just not by adding some $GLOBALS in the template (well for BC reasons obviously I'd still collect them) but instead by the fragment providing them explicitly in an interface method. Also I would abstract it so it's not just returning an array of strings but instead instances of whatever that implement a getLinkTag() or so. Will ease adding stuff like our funny flags we have now (|static|asynchronous etc. pp).

Compiling is a different question and I think it should just be handled like it is now. The page layout settings define whether you want to combine them and where to output them.

To cover the use case of "at the moment any developer can just provide any resource by editing the template", I'd provide some "additional resources" settings in both, the content element and front end module settings :-)

[aschempp]

There's a fundamental issue in your concept. A service must be stateless, and a content element, module, page etc. must know about it's configuration. I don't think you want to pass the config to each method?

[Toflar]

I absolutely don't see any issue in this concept, no.

I don't think you want to pass the config to each method?

Of course! Because we don't want state in these classes. So we'll pass our ModuleModel or ContentModel to our methods (only 2! renderAction() and renderBackendAction()) which essentially is just a POPO containing information so it can be created manually without having a database and thus the elements can be independently rendered as you like.

[aschempp]

The config would be needed for the getResources method as well. And that means you might need to compile the same content twice to figure out the resources…

[discordier]

And that means you might need to compile the same content twice to figure out the resources…

Only for legacy modules to determine the assets (modified part of $GLOBALS.
For new modules only the configuration is examined twice (once in the render() call and another time in the getResources() call.
I like that, as it is separation of concerns.

[aschempp]

You don't know that. As an example, you might need to compile a list of selected files (fetching FilesModel from DB) to determine what scripts you need to load.

[Toflar]

Maybe something like this would work?

public function renderAction(Request $request, ContentModel $entity, ResourceCollection $resources);

Anyway, this is implementation detail already and I'm very happy about your feedback on this because we'll certainly have to clarify this. But can I get some general comments on the approach itself first? Or does discussing the details already mean that it is actually a good way and we should intensify looking at it?

[discordier]

Just some remarks on the current code that jumped into my eyes.
I know it is only a really early preview but I wanted to point them out anyway to not forget them.

[discordier]

I wonder if we should enforce SRP here by only allowing one interface per class.
Either by throwing an exception if multiple present or by breaking here.

[Toflar]

I don't think this is a good idea, we can't add another interface in the future if we want to add new features (which we cannot add to the existing interface due to BC :-)).

[discordier]

We will violate the single responsible approach then and therefore this renders the splitting into multiple "per type" arrays useless?
How shall a class be an insert tag and a content element at the same time? This leads to coupled code with multiple responsibilities.
Can you tell me any example where this is a good idea?

[Toflar]

Ah, no obviously this won't work. But something like ContentElementInterface and SerializableInterface. You mean I should check that only one of the types is implemented?

[discordier]

should be FragmentInterface[]

[discordier]

should be FragmentInterface[]

[discordier]

We should have a lookup map for these too as any fragment can occur more than once (and will as for CE text, headline, ...).

[Toflar]

Will do.

[discordier]

is_a() vs. instanceof?

[Toflar]

We're working on strings, not objects. That's why :)

[discordier]

Anyway, this is implementation detail already and I'm very happy about your feedback on this because we'll certainly have to clarify this.

That's why we are talking about it.

But can I get some general comments on the approach itself first? Or does discussing the details already mean that it is actually a good way and we should intensify looking at it?

Yep, from my side it is confirmed as the way to go, let's move on to the details.

[Toflar]

New big update 😄 Comments on this:

1. I have removed the insert tags stuff for now, It can be easily added later on and it is distracting for this PR. For now I haven't fully implemented content elements and front end modules either. The page type example should illustrate how it works.
2. I have decided to ignore the assets (like css, js etc.) for now, I have some good ideas how we can solve this issue easily but it should be out of scope for this PR because it would make it bigger as it is already.
3. The fragment registry no longer restricts to our core types but provides an abstraction to anyone that wants to ease fragments. Basically, I described it in the code like this:

Fragment registry interface.

This is an abstraction layer for the Symfony fragment handler.
The Symfony fragment handler requires to register real controllers for
every single fragment. In Contao we have different fragment types as well
as fragments themselves (types are modules, content elements etc. names are
e.g. "text", "headline" etc.). It would be very tedious work to register a
controller for every single one of them. This abstraction layer allows you
to tag your service, implement the FragmentInterface (or a subclass of it)
and the rest is taken care of for you.

This means that you can register your own providers now by simply tagging your provider that is implementing the FragmentTypesProviderInterface interface as contao.fragment_types_provider. You can see it in action for our core fragments in the ContaoCoreFragmentTypesProvider. You return an array of fragment types and the appropriate DIC tag and you're done. That way it is very easy to register your own fragment types for stuff like news or products etc. Anything you think makes up a fragment for you :-)

The different fragments can then also very easily be added by using the tag specified in the type provider.
In short: The core bundle provides an abstraction which makes it complex at first sight but automates almost everything for the developers.
Registering a custom page type right now requires the following in the services.yml:

app.custom_page_type:
    class: AppBundle\Controller\PageType\CustomPage
    tags:
        - { name: contao.page_type }

That's it. You now have full control in your class and you benefit from the following:

• You don't have to register any route
• The Symfony fragment URI signer is automatically applied for you
• You can define if you like to render inline or esi or whatever is best for your fragment.

Of course, I've implemented an AbstractPageType so common page type logic is shared :-)
So in the very minimal approach, your CustomPage looks like this:

class CustomPage extends AbstractPageType
{
    /**
     * {@inheritdoc}
     */
    public function getName()
    {
        return 'custom';
    }

    /**
     * {@inheritdoc}
     */
    public function renderAction(Request $request)
    {
        return new Response('So cool, I am rendered inline and I can do whatever I want!');
    }
}

Now, what about some ESI because my stuff can be cached? Requires a lot of configuration? Nope. Simple as that:

class CustomPage extends AbstractPageType
{
    /**
     * {@inheritdoc}
     */
    public function getName()
    {
        return 'custom';
    }

    /**
     * {@inheritdoc}
     */
    public function getRenderStrategy(array $configuration)
    {
        return 'esi';
    }

    /**
     * {@inheritdoc}
     */
    public function renderAction(Request $request)
    {
        $response = new Response();
        $response->setTtl(20); // We can use our proxy cache now!
        return $response;
    }
}

Looking forward to new comments 😎

[discordier]

We should throw an exception here when fragments have already been registered as the new type will not have been evaluated for the existing ones otherwise.
The workflow must be:

1. register types
2. register fragments.

Other solution is to cycle through all existing fragments and evaluate against this type again.

[Toflar]

This can't happen. See addFragment(). If no type is responsible, you cannot add a fragment for it, so you MUST register the type first ;-)

[discordier]

Cleared up on mumble. Adressed in 478b81a

[discordier]

This will need to get splitted in two loops.

1. Register types.
2. Register handlers.

See remarks above for FragmentRegistry::addFragmentType().

[Toflar]

Same comment.

[aschempp]

In Symfony that method is generally called supports(…)

[Toflar]

Yeah which is nonsense here. You register a fragment for a certain type so you don't need to be asked again if you support it.

[aschempp]

I have never seen that in Symfony, maybe you can point to an example where they instantiate a service during container compilation?

[Toflar]

In the YamlFileLoader for example.

[aschempp]

Why not just use is_a? With the third parameter it accepts strings for both arguments.

[Toflar]

is_a() did not check for inherited interfaces for me. Reflection is better here.

[aschempp]

I'm not sure about this thing. It feels rather complicated and a reinvention of something Symfony already provides (the fragment stuff).

Especially on the page types I am still very much a fan of the Symfony CMF / dynamic router because it also includes URL to page alias mapping.

[Toflar]

I'm not sure about this thing. It feels rather complicated and a reinvention of something Symfony already provides (the fragment stuff).

It's not reinventing. It uses the fragment stuff of Symfony. It just maps everything on one controller so you don't have to register it for every type and fragment.

Especially on the page types I am still very much a fan of the Symfony CMF / dynamic router because it also includes URL to page alias mapping.

That has nothing to do with this PR. This PR is about how to render the fragments, not how to find the matching one based on the URL. This is still done in the FrontendIndex. Of course this would still need to be added later on, but this is perfectly possible with this PR.

[aschempp]

I don't think it can be added later if we don't carefully plan it.

The dynamic router had a very sane approach to me. Given a URL find the matching database record (document) and then look for a controller that can return a response based on that document.

We're trying the opposite. Find a "controller" based on an arbitrary name and then setup a configuration that holds something so the "controller" can return a string which is then converted to a response.

[Toflar]

I completely disagree. Nothing of what you're writing is in any way true, I'm sorry. You are talking about mapping an URL like foobar.html to the correct page type. This is something this pull request is not targeting at all and we'll still need to work on. Basically, this is the functionality that's now in FrontendIndex (getPageIdFromUr(), getRootPageFromUrl() etc.). So I don't see how this pull request is in any way related to this issue.
This pull request is building an abstraction layer for fragments of certain types and their respective types again (it's two times type, maybe we can find another key?). So types in terms of "page type", "content element", "front end module" etc. And then obviously types per each type again ("text", "headline", "html" etc.). So it's way more than just your page type use case even though for illustration purposes I've only implemented page types at the moment. The fragment registry can contain anything you want.

I don't want to register a controller for every element that I'm creating. This is pure nonsense. It is even worse if you expect it to map to a database record because I want to render fragments without database entry.

[Toflar]

Okay, I reworked this feature a lot again :) There are no more types and I reduced complexity a lot but still increased flexibility at the same time. I cannot stop emphasizing how powerful this is!
Here's an example of a complex page type, how it would look like now.

Register as a service. As easy as tagging it as a fragment:

app.page_type.service_layer:
    class: AppBundle\Controller\PageType\ServiceLayer
    tags:
        - { name: contao.fragment }

And the class to it which implements all available interfaces, so it's really the most complex version. If you don't want esi you don't implement the RenderStrategyInterface etc. which really reduces things to a minimum.

<?php

namespace AppBundle\Controller\PageType;

use Contao\CoreBundle\Controller\FragmentRegistry\QueryParameterProviderInterface;
use Contao\CoreBundle\Controller\FragmentRegistry\RenderStrategyInterface;
use Contao\CoreBundle\Controller\PageType\PageTypeConfigurationInterface;
use Contao\CoreBundle\Controller\PageType\PageTypeInterface;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpKernel\Exception\NotFoundHttpException;

class ServiceLayer implements PageTypeInterface, RenderStrategyInterface, QueryParameterProviderInterface
{
    /**
     * {@inheritdoc}
     */
    public static function getIdentifier()
    {
        return 'tourismusweb.page_type.service_layer';
    }

    /**
     * {@inheritdoc}
     */
    public function supportsConfiguration($configuration)
    {
        return $configuration instanceof PageTypeConfigurationInterface;
    }

    /**
     * {@inheritdoc}
     */
    public function renderAction(Request $request)
    {
        $pageModel = \PageModel::findByPk($request->query->get('pageId')); // Could be fetched by injecting a service now :-)

        if (null === $pageModel) {
            throw new NotFoundHttpException();
        }

        $msg = <<<MSG
Hi there, I am a page type that was fetched via ESI. I am completely independent and I am only the beginning!
It does not make a lot of sense to render me as ESI because as I am a page type, I'm most likely the root of your
page definition. However, it is possible which shows the flexibility of this concept.
I hope that some day in the future, people will start using fragments a lot more so a page is eventually
made up of loads of different fragments that can be cached individually. We can make Contao extremely
fast then!

So long, your page type "%s".

BTW: I'm cached for 60 seconds. Here's proof:
I was parsed at: %s
MSG;


        $dt = new \DateTime();
        $msg = sprintf($msg, $pageModel->type, $dt->format(\DateTime::ISO8601));
        $response = new Response(nl2br($msg));
        $response->setTtl(60);

        return $response;
    }

    /**
     * {@inheritdoc}
     */
    public function getQueryParameters($configuration = null)
    {
        if (!$this->supportsConfiguration($configuration)) {
            throw new \RuntimeException('You have to provide a configuration I can deal with.');
        }

        /** @var PageTypeConfigurationInterface $configuration */
        return ['pageId' => $configuration->getPageModel()->id];
    }

    /**
     * {@inheritdoc}
     */
    public function getRenderStrategy($configuration = null)
    {
        return 'esi';
    }

    /**
     * {@inheritdoc}
     */
    public function getRenderOptions($configuration = null)
    {
        return [];
    }
}

And here's the output 10 seconds after the first hit: