Automatically set the ajax_load request parameter. #4188
Conversation
|
@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: 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! |
|
@jenkins-plone-org please run jobs |
Zope maintains the "HTTP_X_REQUESTED_WITH" request header. If this is set to "XMLHttpRequest", we have an AJAX request. In this case the ajax_load parameter is set to True directly on the request object. If the ajax_load parameter was already set to a true-ish or false-ish value, it is not overwritten. This should make use for the ajax_main_template for any AJAX request, regardless if the ajax_load parameter was manually set or not and potentially speed up Plone Classic UI rendering.
|
The related PRs seem fine, and could be merged and used in 6.1, even without the current PR, in my opinion. But I have a doubt about this one for Plone 6.1. You already saw some unexpected problem in What do others think? |
| checkPermission python:context.restrictedTraverse('portal_membership').checkPermission; | ||
| ajax_include_head python:request.get('ajax_include_head', False); | ||
| ajax_load python:False;" | ||
| ajax_load python:True;" |
There was a problem hiding this comment.
I have no idea why this ajax_main_template has ajax_load=False in the first place. This variable seems really obscure defined like this.
There was a problem hiding this comment.
Agree.
But I think it's maybe needed by other templates and was incorrectly defined due to a copy paste error.
I did now looked it up and saw:
- It was added here: 4f58ba6
- The copy error changing from
ajax_load python:Truetoajax_load python:Falsehappened in this commit at some Barceloneta LTS syncing:
954942a
Now it makes sense.
But no big deal apparently that it was defined incorrectly, because everything was working just fine.
| if ( | ||
| request.getHeader("HTTP_X_REQUESTED_WITH") == "XMLHttpRequest" | ||
| and not request.get("ajax_load", MARKER) is not MARKER | ||
| ): | ||
| # Directly set on the request object | ||
| request.set("ajax_load", True) |
There was a problem hiding this comment.
I do not like this approach.
You are checking the request, fiddling with the request parameters, and then checking the request again.
IMO this is too complicated.
There should be some come somewhere that has a method is_ajax_load (or whatever other name) that is the unique source of truth about the fact that we are loading stuff through AJAX.
Ideally, this should be a method in a view (plone_context_state?) so that it is easily configurable (e.g. you might want to always load some portal type as if it was called as AJAX, add a check for the referrer or whatever).
BTW even if you want to have this traverser, the second condition has to be simplified because it contains a double negation.
Something like:
request.get("ajax_load", MARKER) is MARKERor even:
"ajax_load" not in requestis preferable.
There was a problem hiding this comment.
LOL, and not request.get("ajax_load", MARKER) is not MARKER .... yes, i'll take your last suggestion - this is much better 😄
There was a problem hiding this comment.
Regarding the other suggestion. Sounds good, but I always saw the ajax_load request parameter as the source of truth to set something in ajax mode. In the end, the Diazo template uses it. In Plone 5 and in Plone 6 since since plone/plonetheme.barceloneta#404
Also some Plone links are configured with ajax_load, IIRC.
For that, we need to set it early, I think - therefore the IBeforeTraverse event subscriber here.
Also, we could disable Diazo via another http header here. For example, if a registry entry for disabling diazo is set to True , we could set the header X-Theme-Disabled and disable Diazo that way easily for everyone. ... for a future addition.
|
@ale-rt I have addressed your comments in #4169 I agree with @mauritsvanrees this should probably not be merged into Plone 6.1 but 6.2 instead. |
4 participants
Related:
#4188
plone/plone.base#85
plone/plonetheme.barceloneta#404
plone/plone.app.portlets#203
Plone 6.1: #4188 (this one)
Plone 6.2: #4169
Zope maintains the "HTTP_X_REQUESTED_WITH" request header. If this is set to "XMLHttpRequest", we have an AJAX request. In this case the ajax_load parameter is set to 1 directly on the request object.
This should make use for the ajax_main_template for any AJAX request, regardless if the ajax_load parameter was manually set or not and potentially speed up Plone Classic UI rendering.