Add RFC 007, PageConfigs

[mx-moth]

[kaedroho]

Rendered

Could we renumber this to 7 to match the pull request number? I know this might introduce confusing gaps but I think it would be nice to have an easy way to get back to the initial discussion from the RFC number.

Also, cheers for making an RFC!

[mx-moth]

I've renumbered it, as suggested.

[gasman]

I agree this aspect of Wagtail is badly in need of a clean-up - thanks for tackling it @timheap!

I like the idea of 'wrapping' models in helper classes that provide the configuration for a particular problem domain, instead of cramming the base model with peripherally-relevant settings and methods - like django.contrib.admin does with ModelAdmin classes, and Django Rest Framework does with serializers. We should absolutely do more of that. (I'm sure the design pattern folks must have a name for this - I really should read the Gang Of Four book some day...)

Do we gain anything from having a central PageConfig registry for these things, rather than each module defining their own distinct wrapper? (It seems to me that PageConfigs are liable to get just as bloated as Page models do at the moment.) I could imagine a setup where each app has a search.py containing ModelSearch classes (where search_fields definitions are kept), a sitemap.py containing ModelSiteMap classes (where get_sitemap_urls methods are defined), a wagtailadmin.py containing PageAdmin classes (where content_panels definitions live)...

There are a few things I'm reluctant to offload to a wrapper class - to me subpage_types and parent_page_types are fundamentally model-level properties. While all the things I've talked about above (API, search, sitemap, Django admin, Wagtailadmin) are effectively different views on the data model, subpage_types / parent_page_types are part of the data model - business rules about how the page tree is structured (which wagtailadmin, the API etc are in turn expected to adhere to).

Probably the toughest decision for me here is what to do about the whole page template rendering infrastructure. The principle of Page being "a model that knows how to render itself in response to requests" was basically the whole premise Wagtail was built on - yes, it does break MVC, and that was a conscious design choice, on the basis that a content-managed website is not the same thing as a conventional web app (in a web app, views exist in fixed locations; on a content-managed site, they move around, so it's appropriate for our 'views' to actually be database objects). That doesn't mean we have to set that design in stone, though... The other benefit of wrapper objects (besides splitting up the code) is the ability to have multiple wrappers for the same model - something often seen in DRF's serializers, where you often want 'summary' and 'full' serialisations of the same model - and that's something that could be very relevant here, allowing multiple renderings of a page in line with the whole "create once publish everywhere" philosophy.

[mx-moth]

I agree that individual configurations would be technically cleaner, but I don't think it strikes the right balance between technical purity and developer usability. Consider the configuration for a searchable, sitemappable, static-generatable page:

from .models import MyPage

@register_page_config(MyPage)
class MyPageConfig(PageConfig):
    subpage_types = []
    base_form_class = MyPageForm
    content_panels = [
        # ...
    ]

@register_search_config(MyPage)
class MyPageSearchConfig(SearchConfig):
    search_fields = [
        # ....
    ]

@register_sitemap_config(MyPage)
class MyPageSitemapConfig(SitemapConfig):
    def get_sitemap_urls(self, page):
        return ['/', '/foo/', '/bar/']

And then repeat this for another few pages. I think we will loose a considerable amount of convenience and developer friendliness that currently exists with the way Wagtail groups everything together. Split across multiple files, and the sheer number of files with tiny snippets of configuration would become unmanageable in a different way. It would also make interactions between different mixins difficult. Consider a RoutablePage that knew how to automatically fill out get_sitemap_urls, for example. Finally, sharing any common code for a Page type between multiple configuration classes would be difficult.

If you swap how you think about the models vs. configurations, I think I can convince you that the PageConfig class is the correct place for the parent_page_types/subpage_types. Currently, the Page model class is the sole source of truth about what a Page is. This proposal turns this on its head completely, making the PageConfig class what makes a Page a Page. The model just happens to be where the data for a Page instance is stored. The PageConfig defines all the behaviour of how a Page interacts with the Wagtail system, including where Wagtail should look to find data for this Page type. The model becomes the secondary class, and the PageConfig is the real deal.

This then makes it clear where the rendering functionality should live: on the PageConfig, which is what makes a Page a Page. The model merely stores the data for the PageConfig to use later when needed. It also adheres more to the MVC ideal, and opens up the possibility of multiple renderings for the same underlying datastore.

[balazs-endresz]

On a sidenote, I feel like the Proxy page models RFC https://github.com/torchbox/wagtail/pull/1736 is meant to solve some of the same issues: i.e. having a separate entity in the admin that renders the page with a different template, without duplicating the data definition. But instead of registering a page config, it works by subclassing a page. Although, I guess it doesn't make things much more MVC-like.