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.
Files: 
CommentFileSizeAuthor
#18 take_get_request_into_account-2497115-18-complete.patch4.47 KBmelvinlouwerse
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,494 pass(es). View
#18 take_get_request_into_account-2497115-18-tests.patch3.47 KBmelvinlouwerse
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,926 pass(es), 3 fail(s), and 0 exception(s). View
#12 take_get_request_into_account-2497115-13.patch4.44 KBmelvinlouwerse
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,965 pass(es). View
#8 take_get_request_into_account-2497115-8.patch3.05 KBmelvinlouwerse
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch take_get_request_into_account-2497115-8.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

Fabianx’s picture

Status: Needs work » Active
Crell’s picture

Where is this alleged code...?

Fabianx’s picture

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

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

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

Sure, but in core that never happens.

dawehner’s picture

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

FileSize
3.05 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch take_get_request_into_account-2497115-8.patch. Unable to apply patch. See the log in the details link for more information. View

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

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

FileSize
4.44 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,965 pass(es). View

fix the patch

melvinlouwerse’s picture

Status: Needs work » Needs review
DuaelFr’s picture

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

Status: Needs review » Needs work
Wim Leers’s picture

Assigned: Fabianx » Unassigned
Issue tags: +Novice, +php-novice, +DrupalCon Barcelona 2015

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

melvinlouwerse’s picture

Assigned: Unassigned » melvinlouwerse
melvinlouwerse’s picture

FileSize
3.47 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,926 pass(es), 3 fail(s), and 0 exception(s). View
4.47 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,494 pass(es). View

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

melvinlouwerse’s picture

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

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

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

Status: Reviewed & tested by the community » Fixed

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

claudiu.cristea’s picture

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.