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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia commented
Status: Needs work » Active
Crell’s picture

Crell CreditAttribution: Crell at Palantir.net commented

Where is this alleged code...?

Fabianx’s picture

Fabianx CreditAttribution: Fabianx for Acquia 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 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia 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 CreditAttribution: Fabianx for Acquia 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 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Sure, but in core that never happens.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Tag1 Consulting 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 CreditAttribution: melvinlouwerse as a volunteer and at Ibuildings commented
FileSize
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 CreditAttribution: melvinlouwerse as a volunteer and at Ibuildings 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 CreditAttribution: melvinlouwerse as a volunteer and at Ibuildings commented
FileSize
take_get_request_into_account-2497115-13.patch4.44 KB

fix the patch

melvinlouwerse’s picture

melvinlouwerse CreditAttribution: melvinlouwerse as a volunteer and at Ibuildings commented
Status: Needs work » Needs review
DuaelFr’s picture

DuaelFr
he/him
Primary language French
Location Montpellier, France
CreditAttribution: DuaelFr as a volunteer and at Happyculture 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
CreditAttribution: DuaelFr as a volunteer and at Happyculture commented
Status: Needs review » Needs work
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia 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 CreditAttribution: melvinlouwerse as a volunteer and at Ibuildings commented
Assigned: Unassigned » melvinlouwerse
melvinlouwerse’s picture

melvinlouwerse CreditAttribution: melvinlouwerse as a volunteer and at Ibuildings commented
FileSize
take_get_request_into_account-2497115-18-tests.patch3.47 KB
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 CreditAttribution: melvinlouwerse as a volunteer and at Ibuildings 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 CreditAttribution: Fabianx for Acquia 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
CreditAttribution: legolasbo 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 CreditAttribution: effulgentsia at Acquia 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 🇷🇴
CreditAttribution: claudiu.cristea at Webikon 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.