Accept page change in before_serve_page handling

[robmoorman]

Is your proposal related to a problem?

Currently the before_serve_page accepts HttpResponse's in order to accept adjusted page content. For headless setups (and in theory other situations) you maybe don't want to return Html but Json instead. However you still want to benefit from the before_serve_page hooks mechanisme.

Describe the solution you'd like

Change the page instance in a before_serve_page rather than accepting HttpResponse's.

Describe alternatives you've considered

I tried to re-build the serve method, however you're dependent of the implementation in modules (e.g. wagtail-personalization) which are using the before_serve_page hooks in order to apply their business logic.

Additional context

Code example for the solution:

for fn in hooks.get_hooks('before_serve_page'):
    result = fn(page, request, args, kwargs)
    if isinstance(result, HttpResponse):
        return result
    if isinstance(result, Page):
      page = result

return page.serve(request, *args, **kwargs)

[thibaudcolas]

Related: #11752.

[gasman]

I like this idea! Sorry I didn't notice this before, @robmoorman...

Marking this as a good first issue. As additional background for anyone interested in pick this up - see the documentation for the before_serve_page hook. This change means that you'd be able to adjust the example code as given there into:

from wagtail import hooks
from myapp.models import BlogPage

@hooks.register('before_serve_page')
def block_googlebot(page, request, serve_args, serve_kwargs):
    if request.META.get('HTTP_USER_AGENT') == 'GoogleBot':
        # serve the "information for robots" page in place of the one requested
        return BlogPage.objects.get(slug="information-for-robots")

The code that @robmoorman has provided would go into the serve view. You should verify that this works as intended (and fix it if not), and add a unit test to Wagtail's test suite to demonstrate it working - I'd suggest adding a new before_serve_page hook function to wagtail/test/testapp/wagtail_hooks.py alongside the one that already exists, and a test method in wagtail/tests/test_page_model.py alongside test_before_serve_hook. Finally, the hooks reference documentation will need to be updated to mention the option of returning a page object.

[ababic]

With the page-swapping examples... Does the final page not also need to be passed through the relevant hooks before it is served?

[gasman]

@ababic I don't think so - can you expand on what you have in mind?

Perhaps you're thinking along the lines of what happens to request objects inside middleware, where successive middleware classes contribute properties like request.session and request.user, and swapping out the request object partway through that is likely to result in one with missing properties? I don't think that'll be an issue for pages, since there's not really any reason for hooks to modify the state of the page object.

[akshatggupta]

@gasman I want to work on this issue...

[gasman]

@akshatggupta Excellent! See https://docs.wagtail.org/en/latest/contributing/index.html for some pointers on getting started.

[ababic]

@gasman Apologies for the delayed response. My github notifications are somewhat polluted with client project notifcations currently.

Can you expand on what you have in mind?

Yes, what I mean is: because pages have no equivalent setup() method like views do, which is always reliably called as part of the rendering process... it could be that developers are using the before_serve_page hook to similarly 'prepare' page objects in a specific way before they are rendered. If you swap the page out for another one in a before_serve_page hook (with the intention of a different page being 'served'), and do not run before_serve_page again for that page, are you not breaking an existing contract (before_serve_page() will always be called for a page before it is served)?

[gasman]

@ababic Ah yep, that's a good point - and from a quick survey of public code on Github using @hooks.register('before_serve_page'), it's not taken too long to find a real-world case that could theoretically be affected by this - a prep_footnotes hook that writes to the page's footnotes attribute.

All the same, I think it would be sufficient to document the fact that the ordering of before_serve_page hooks may be significant, and to consider setting an explicit order when registering the hook - in much the same way that Django middleware classes may need to be run in a particular order to work correctly. Arguably this is already the case in some senses - for example, if you have before_serve_page hooks for both permission checking and hit counting, then the hit counting hook has to run later to avoid over-counting.

[ababic]

@gasman In all honesty, I've got a bad feeling about doing this in before_serve_page all together. I don't think it's the correct part of the process to tinker with. The requirements sound like 'custom routing' for those who don't want to have to understand how page routing works. If it must be hook-powered, then I think a new hook should be added for that specific purpose, e.g. "swap_page", then just let before_serve_page run it's full course after. What do you think?

The example above would simply become:

@hooks.register('swap_page')
def block_googlebot(page, request, serve_args, serve_kwargs):
    if request.META.get('HTTP_USER_AGENT') == 'GoogleBot':
        # serve the "information for robots" page in place of the one requested
        return BlogPage.objects.get(slug="information-for-robots")

If the hook returns None, it simply continues to serve page.

[akshatggupta]

Hey @ababic I'm trying to work on this issue what do you think about this approach

from wagtail import hooks
from myapp.models import BlogPage

@hooks.register('before_serve_page')
def block_googlebot(page, request, serve_args, serve_kwargs):
    if request.META.get('HTTP_USER_AGENT') == 'GoogleBot':
        # Fetch the alternative page object
        try:
            alternative_page = BlogPage.objects.get(slug="information-for-robots")
        except BlogPage.DoesNotExist:
            return None  # If the page doesn't exist, fallback to default handling
        
        # Return the alternative page object
        return alternative_page
    
    # For other user agents, continue with normal behavior
    return None

If the page exists, it is returned directly.
Wagtail treats this as the page to be served, bypassing the original page.
If the page with the slug information-for-robots doesn't exist, or if the user agent doesn't match, the hook returns None, and Wagtail serves the requested page normally.

[ababic]

@akshatggupta I'm suggesting that if the page needs to be swapped, that be done via a separate swap_page hook before on_serve_page or before_serve_page hooks are run/applied.

[gasman]

When I gave the thumbs-up to this, it seemed like a quick win and a natural extension of the before_serve_page hook - but as @ababic has pointed out, there's potential for this to break existing setups where the hook functions perform some kind of state change on the page object, such as setting an attribute that the page view then expects to be able to access. For that reason, it makes sense to take a step back and weigh up the potential benefits of this change against the risks.

@robmoorman, can you give more detail about your planned use-case for this? Does this achieve anything that couldn't be done by setting a custom route method on the site's root page?