Automatically set the ajax_load request parameter.

[thet]

Related:
#4169
plone/plonetheme.barceloneta#404

Plone 6.1: #4188 not addressing 6.1 anymore. Review comments addressed here.
Plone 6.2: #4169 (this one)

Zope maintains the "HTTP_X_REQUESTED_WITH" request header. If this is set to "XMLHttpRequest", we have an AJAX request. If so, the ajax_load parameter is set to "true", regardless if it was set manually or not.

This should make use for the ajax_main_template for any AJAX request, potentially speeding up classic Plone by avoiding loading unnecessary stuff.

Update: Added @davisagli and @ale-rt for more opinions on that.

[mister-roboto]

@thet thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[thet]

@jenkins-plone-org please run jobs

[mauritsvanrees]

This seems fine to me.

If I see it correctly, the related PRs could each be tested and merged separately (once any comments there are addressed). That makes it easier to test this one as well.

[thet]

@jenkins-plone-org please run jobs

[davisagli]

I think you have the wrong understanding of where X-Requested-With comes from.

Aside from that, this looks okay as long as there's never a case where jquery is making a request where the entire page html is actually needed. (But in that case, there's at least the workaround of passing ?ajax_load=0)

[thet]

@davisagli Thanks for the answer - I felt I was missing out something you might know :)

Right, I see, that X-Requested-With is not Zope maintained.
I will chew a bit more on this.

The X-Requested-With is non-standard but commonly used. jQuery sends it automatically. Since we have control over Patternslib and Mockup we can send that header also with fetch.

That will already cover most cases.

And if a AJAX request is done without this header, nothing will break - only the whole page rendered.

And yes, if needed this behavior can explicitly be turned off with ajax_load=0

[davisagli]

(Side note: when you force-push a squashed commit, it makes it hard to see what changed compared to previous commits that I already reviewed. I'd rather you add new commits until the PR is done, and then use the "squash and merge" button to merge it as a single commit.)

Edited to add: I see github provides a "compare" button to see the changes, so maybe this is just a personal preference thing.

[thet]

Here some more info on X-Requested-With. It seems to be a common de-facto standard to use X-Requested-With=XMLHttpRequest to indicate AJAX requests. Still - there is no guarantee that the header is present - especially fetch doesn't automatically send the header automatically. However, fetch can be wrapped/patched.

• Pylon's WebOb uses X-Requested-With to detect AJAX requests: https://github.com/Pylons/webob/blob/39d5af3c797e7b867f152c2e8c979de42d029403/src/webob/request.py#L644
• https://en.wikipedia.org/wiki/List_of_HTTP_header_fields#Requested-With - Wikipedia mentions the X-Requested-With as common but non-standard header.
• https://api.jquery.com/jQuery.ajax/ - header automatically added for jQuery.ajax requests.
• X-Requested-With request header is not set with XMLHttpRequest JakeChampion/fetch#17 - header not present in fetch and won't be.
• https://gist.github.com/dgraham/92e4c45da3707a3fe789 - Gist from a comment from the previous link showing a fetch wrapper which adds X-Requested-With.
• Add missing Request.IsAjaxRequest aspnet/HttpAbstractions#245 - Discussion for ASP.net
• https://stackoverflow.com/a/76370831 - Comment suggesting that the new sec- headers could be used to detect AJAX requests. Not sure, if that holds true - a first research didn't look so.
• https://www.stoutner.com/the-x-requested-with-header/ - Mentions another use of X-Requested-With in Androids WebView component.

[ale-rt]

See my comments on the sister PR #4188 (comment)

I am not blocking this, but IMO this is too indirect and potentially will make it hard to debug what is happening.

Keep also in mind that subscribers are also hard to override.

I would very much prefer to have the check for "is ajax loaded" into a view method.
That method could look for all the headers and request parameters you want.

[davisagli]

@thet I just saw the change in plone/plonetheme.barceloneta#404 to stop applying Diazo theming if ajax_load is true. This doesn't seem safe to me. It could result in different markup if the html for a page is loaded entirely, and then part of the page is replaced to update it by making the same request via an ajax request.

Keep also in mind that subscribers are also hard to override.
I would very much prefer to have the check for "is ajax loaded" into a view method.

This sounds like a good idea to me.

[thet]

@davisagli @ale-rt

Disabling Diazo for any AJAX request was really the intention. That is what I would expect - lean and fast responses for AJAX with all the information I need and without the overhead of Diazo and a full-blown main template. In my opinion Diazo should not be needed for AJAX requests, and for the edge cases where it is needed there is always ajax_load=0 request parameter.

But I could be wrong with my "anyone wants to disable diazo for ajax" assumption here :)

With this PR we have the same situation also for the main template. Before the ajax_load=1 parameter would have explicitly to be set to not render the whole Plone page when only small AJAX updates were needed. Now only the main content is rendered.

Regarding the IBeforeTraverseEvent subscriber - ajax_load (used already since at least Plone 4 to strip down the main template and disable Diazo for those who used it back then)‌ is a request parameter. And as such I want to have it set as early as possible when automatically set. Therefore I use the event subscriber here and not a browser view or utility which would get called after traversing.

Maybe I should set it even earlier, like in a subscriber for ZPublisher.interfaces.IPubStart?

But if this auto-set feature seems to be too dangerous - which I disagree with - I see these options:

• Really using an easily overridable browser view to ONLY do the main template switch and not disable Diazo at all.
• Making an optional feature switch which can be enabled in the theming controlpanel which would set the ajax_load parameter as early as possible (not a browser view, but again something like this event subscriber from this PR here resp ZPublisher.interfaces.IPubStart)
• Making a PLIP out of this, scheduled for Plone 7
• Dropping this idea as a whole and maybe creating an add on instead.

I'd love to hear the opinion of other Classic UI people doing a lot of frontend stuff. /cc @MrTango @petschki @pbauer

[thet]

Update: Updated for latest changes from #4196

[thet]

@jenkins-plone-org please run jobs

[thet]

@jenkins-plone-org please run jobs

[ale-rt]

Maybe I should set it even earlier, like in a subscriber for ZPublisher.interfaces.IPubStart?

IMO it is the opposite, you should not set it.

As explained earlier:

1. You are checking the request
2. You are fiddling with the request
3. You are re-checking the request

Then you do something.

My proposal is to strip down that approach to:

1. You check the request in a view method

Then you do something.

I also think that you should do that in a Plone classic only package.

But if this auto-set feature seems to be too dangerous - which I disagree with

I do not see any danger at all, it just seems to me too cumbersome to do this 3-steps process when you can do that in one step only.

[davisagli]

@thet I definitely understand the desire to avoid rendering the entire main_template for AJAX requests. And then I can see that it becomes necessary to disable Diazo, because the Diazo rules are probably expecting the full page structure from main_template.

But my concern is that since the markup may not be the same if Diazo rules are not applied, then some CSS or Javascript may not find the classes or ids that it is looking for.

This is probably not a big problem in the Barceloneta theme, since it makes minimal changes to the unthemed markup. So maybe the change is okay there. And I definitely won't block this myself, since I am not usually working with Classic UI sites these days. But I wanted to explain my concern.

[thet]

@davisagli tnx. I understand these concerns and this discussion made me rethink this PR‌ myself. I think the safest option would be to make this optional, with the default turned off.

I tried yesterday to make this configurable, but I cannot get the reqistry via getUtility(IRegistry) at ZPublisher.interfaces.IPubStart time, not even for zope.traversing.interfaces.IBeforeTraverseEvent (although I would have the portal as object and could just hardcode portal.portal_registry to access the registry, but I don't like that). ZPublisher.interfaces.IPubAfterTraversal would work, but then the main template was already factored and this is too late.

Even when I really thought I need/want to set this request parameter as early as possible technical limitations draw me towards the browser view approach, @ale-rt already suggested :)

Or an add-on. Not sure what is a better fit.

[davisagli]

@thet Ok, I feel better about it if it is off by default.

[thet]

@ale-rt I understand now what you mean by checking the request, fiddling with it and checking it again.

This was a very valuable discussion for me - thanks @ale-rt and @davisagli! The last iteration of this code benefited from it and is IMO really much better (it's not ready yet).

My premise was to set ajax_load for everyone to consume. Now this is not the case, which is a drawback, but I think this doesn't matter much. Especially with the is_xhr method anyone is invited to use it, but there is maybe not so much use for it.... Nonetheless, the is_xhr should probably really go into plone context state. Currently it's just directly in the main template class.

For the Diazo part, I plan to do something similar - creating a config option to automatically disable it for AJAX requests to save processing power and save from Diazo shenanigans.

Is the site control panel the right place for the use_ajax and a future ajax_disable_diazo option? Are the option names OK?

See:
plone/plone.base#87
plone/plone.app.upgrade#345

[thet]

Btw. when enabling, the ajax main template is not used very much. Currently I just saw it used for only for the rename dialog :D

But! I experimented a bit with automatic XHR requests for any internal link within a site for some projects with quite promising results. This can achieved by something similar like:

import pat_inject from "@patternslib/patternslib/src/pat/inject/inject";
import registry from "@patternslib/patternslib/src/core/registry";

// Add a new trigger for the inject pattern.
// NOTE: This uses pat-inject AJAX loading for all internal links with exceptions.
registry.patterns.inject.trigger = `${registry.patterns.inject.trigger}, .pat-inject a[href^="${window.location.origin}"]:not(.dropdown-toggle):not([id^="autotoc"])`;

and in html:

<body class="pat-inject"
      data-pat-inject="
          source: main::element;
          target: main::element;
          history: record &&
          source: head title;
          target: head title &&
          source: #language_navigation::element;
          target: #language_navigation::element &&
          source: head link[rel=canonical]::element;
          target: head link[rel=canonical]::element;
      "
>

[davisagli]

Is the site control panel the right place for the use_ajax and a future ajax_disable_diazo option? Are the option names OK?

ajax_disable_diazo sounds like something that should be part of IThemeSettings in https://github.com/plone/plone.app.theming/blob/master/src/plone/app/theming/interfaces.py#L89

[petschki]

Sorry for being late here. I like the final approach with is_xhr and ajax_load but as you said, I think it would make definitely more sense to bring those methods to plone.app.layout.globals.portal.

But using them in CMFPlone's main_template view brings us back to the complexity of separating Classic-UI from core ... consequently plone.app.layout should have a customized @@main_template view (we have now a browserlayer there) with those new methods implemented.

And I'd also put the configuration options to the IThemeSettings interface.

[petschki]

import registry from "@patternslib/patternslib/src/pat/inject/inject";
import registry from "@patternslib/patternslib/src/core/registry";

// Add a new trigger for the inject pattern.
// NOTE: This uses pat-inject AJAX loading for all internal links with exceptions.
registry.patterns.inject.trigger = `${registry.patterns.inject.trigger}, .pat-inject a[href^="${window.location.origin}"]:not(.dropdown-toggle):not([id^="autotoc"])`;

Classic-UI goes SPA in 3 lines of javascript ... I like! 😄