Follow-up to #2728815: Batch API uses request attributes instead of query in Symfony 3

Problem/Motivation

The semantics of Request::get() have changed in Symfony 3. Request attributes are now examined first before request query parameters.

Let's do it in 2 phases: commit this small patch first as soon as it passes to make progress with the Symfony 3 update. Phase 2: replace all remaining $request->get() usages.

Remains are

core/modules/language/src/Plugin/LanguageNegotiation/LanguageNegotiationContentEntity.php:94:    $langcode = $request->get(static::QUERY_PARAMETER);
core/modules/search/src/Controller/SearchController.php:82:      $keys = trim($request->get('keys'));
core/modules/system/src/Controller/ThemeController.php:63:    $theme = $request->get('theme');
core/modules/system/src/Controller/ThemeController.php:105:    $theme = $request->get('theme');
core/rebuild.php:41:  ($request->get('token') && $request->get('timestamp') &&
core/rebuild.php:42:    ((REQUEST_TIME - $request->get('timestamp')) < 300) &&
core/rebuild.php:43:    Crypt::hashEquals(Crypt::hmacBase64($request->get('timestamp'), Settings::get('hash_salt')), $request->get('token'))
core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php:104:    $request->get('ajax_iframe_upload', FALSE)->shouldBeCalled();
core/tests/Drupal/Tests/Core/StackMiddleware/NegotiationMiddlewareTest.php:129:    $request->get('ajax_iframe_upload', FALSE)->shouldBeCalled();

Proposed resolution

Explicitly call $request->query->get() in the Batch API to only access URL query parameters and never request attributes.

Remaining tasks

Patch.

User interface changes

None.

API changes

None.

Data model changes

None.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost created an issue. See original summary.

andypost’s picture

Status: Active » Needs review
FileSize
4.7 KB

Looks only NegotiationMiddleware tests should be fixed, let's see other failures

Status: Needs review » Needs work

The last submitted patch, 2: 2729247-2.patch, failed testing.

andypost’s picture

Looks we need functional test for ajax_iframe_upload

jibran’s picture

andypost’s picture

Status: Needs work » Needs review
FileSize
2.63 KB
6.63 KB

Fix failed tests, ajax_iframe_upload passed as POST data in case of iframe uploads for old browsers so this testing out of scope

klausi’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, tested that there are no remaining $request->get() usages left:

ag "[^>]request->get\("

Yields no results with the patch. RTBC, assuming the testbot agrees.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 6: 2729247-6.patch, failed testing.

klausi’s picture

Status: Needs work » Reviewed & tested by the community

Random test fail I guess? Back to RTBC.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.2.x, thanks!

  • catch committed 240586c on 8.2.x
    Issue #2729247 by andypost: Replace remaining $request->get() usage to...

Status: Fixed » Closed (fixed)

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