Follow-up to #2407195: Move attachment processing to services and per-type response subclasses

Problem/Motivation

       $ajax_page_state = $this->requestStack->getCurrentRequest()->request->get('ajax_page_state');

is wrong.

Proposed resolution

Should be:

       $ajax_page_state = $this->requestStack->getCurrentRequest()->get('ajax_page_state');

but needs tests coverage.

Remaining tasks

- Fix it
- Write tests
- Commit

User interface changes

- None

API changes

- None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because setting ajax page state via GET is broken.
Issue priority Major because it renders a turbo links approach that was to be supported useless for no reason.
Prioritized changes The main goal of this issue is to fix a bug to allow performance (less data to load) improvements down the road.
Disruption Not disruptive at all.
CommentFileSizeAuthor
#18 take_get_request_into_account-2497115-18-complete.patch4.47 KBmelvinlouwerse
#18 take_get_request_into_account-2497115-18-tests.patch3.47 KBmelvinlouwerse
#12 take_get_request_into_account-2497115-13.patch4.44 KBmelvinlouwerse
#8 take_get_request_into_account-2497115-8.patch3.05 KBmelvinlouwerse

Comments

fabianx’s picture

fabianx commented
Status: Needs work » Active
Crell’s picture

Crell commented

Where is this alleged code...?

fabianx’s picture

fabianx commented

Currently template_preprocess_html() and it looks different:

$ajax_page_state = \Drupal::request()->request->get('ajax_page_state');

should be:

$ajax_page_state = \Drupal::request()->get('ajax_page_state');

What is in the IS is after that other issue is in ...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

All AJAX requests are POST requests. (Yes we are working towards making AJAX GET requests possible too, but we're not there yet.)

Therefore I don't see how we ever can hit this bug. Shouldn't this therefore be normal?

But yes, it is a bug.

fabianx’s picture

fabianx commented

Nope, this is especially there to support rails turbolinks, so for any request to e.g. /node/?ajax_page_state=drupal/drupal

you tell the server, which libraries you have loaded already and it then just loads the remainder, etc.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Sure, but in core that never happens.

dawehner’s picture

dawehner
Primary language German
commented

Sure, at some point we also want to switch to GET for normal ajax request but still keep support for ajax forms, which will be POST.

melvinlouwerse’s picture

melvinlouwerse commented
StatusFileSize
new take_get_request_into_account-2497115-8.patch3.05 KB

Not sure if i put the tests in the right directory but as the changed code is in core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php i added it to the directory for Render test. If this is not the correct directory let me know where i should move it to please.

melvinlouwerse’s picture

melvinlouwerse commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 8: take_get_request_into_account-2497115-8.patch, failed testing.

The last submitted patch, 8: take_get_request_into_account-2497115-8.patch, failed testing.

melvinlouwerse’s picture

melvinlouwerse commented
StatusFileSize
new take_get_request_into_account-2497115-13.patch4.44 KB

fix the patch

melvinlouwerse’s picture

melvinlouwerse commented
Status: Needs work » Needs review
duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Issue tags: -Twig, -Novice, -Needs tests, -php-novice

Thank you @melvinlouwerse for this work!
I have two comments, though.

First, I think you should keep using the injected requestStack service (as written in the issue summary) because it makes the method more testable and it also avoids an unneeded call to the container.

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -277,8 +277,7 @@ protected function processAssetLibraries(array $attached, array $placeholders) {
-    $ajax_page_state = $this->requestStack->getCurrentRequest()->request->get('ajax_page_state');
+    $ajax_page_state = \Drupal::request()->get('ajax_page_state');

Also, could you please also post a test-only patch to be sure that the existing code is failing? See that documentation on how to write tests that includes that part: https://www.drupal.org/contributor-tasks/write-tests

duaelfr’s picture

duaelfr
he/him
Primary language French
Location Montpellier, France
commented
Status: Needs review » Needs work
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Assigned: fabianx » Unassigned
Issue tags: +Novice, +php-novice, +DrupalCon Barcelona 2015

@melvinlouwerse Feel free to assign the issue to yourself :)

melvinlouwerse’s picture

melvinlouwerse commented
Assigned: Unassigned » melvinlouwerse
melvinlouwerse’s picture

melvinlouwerse commented
StatusFileSize
new take_get_request_into_account-2497115-18-tests.patch3.47 KB
new take_get_request_into_account-2497115-18-complete.patch4.47 KB

Sorry i indeed seem to have copied the wrong piece of code (from a comment not from the summary).

melvinlouwerse’s picture

melvinlouwerse commented
Status: Needs work » Needs review

The last submitted patch, 18: take_get_request_into_account-2497115-18-tests.patch, failed testing.

The last submitted patch, 18: take_get_request_into_account-2497115-18-tests.patch, failed testing.

fabianx’s picture

fabianx commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Hah, so great to see an issue get done while away!

Thanks so much @melvinlouwerse

RTBC, nice work on the tests!

--

Added beta evaluation.

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
commented
Assigned: melvinlouwerse » Unassigned

RTBC +1

  • effulgentsia committed 0dd08e6 on 8.0.x 
    Issue #2497115 by melvinlouwerse: ajax_page_state is not taken into...
effulgentsia’s picture

effulgentsia commented
Status: Reviewed & tested by the community » Fixed

Yay, nice to see this fixed! Pushed to 8.0.x.

claudiu.cristea’s picture

claudiu.cristea
Primary language Romanian
Location Arad 🇷🇴
commented

This is causing errors on the old bot. Followup #2579965: AssertContentTrait "use"d twice in AjaxPageStateTest.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

quietone’s picture

quietone commented
Issue tags: -, -DrupalCon Barcelona 2015 +Barcelona2015