Part of #1998638: Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object

Files needing conversion:

  • core/modules/block/block.module
  • core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockFormController.php
  • core/modules/book/book.module
CommentFileSizeAuthor
#8 1998708-block-request-8.patch2.42 KBkim.pepper
#4 1998708-block-request-4.patch19.25 KBkim.pepper
#1 1998708_1.patch2.46 KBkmcculloch

Comments

kmcculloch’s picture

kmcculloch commented
Status: Active » Needs review
StatusFileSize
new 1998708_1.patch2.46 KB

Initial attempt to convert, using (what I believe are) proper Symfony methods

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented 
+++ b/core/modules/block/block.moduleundefined
@@ -315,7 +315,7 @@ function _block_get_renderable_region($list = array()) {
+    !in_array(\Drupal::request()->server->get('REQUEST_METHOD'), array('GET', 'HEAD'));

This could just be $request->isMethod('GET') || $request->isMethod('HEAD')

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs review » Needs work
kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 1998708-block-request-4.patch19.25 KB

There is now only one occurrence of PHP superglobals, so I started this again from scratch. I've used a $request-isMethodSafe() to encapsulate the same check.

Status: Needs review » Needs work

The last submitted patch, 1998708-block-request-4.patch, failed testing.

Steven Merrill’s picture

Steven Merrill commented
Status: Needs work » Needs review

#4: 1998708-block-request-4.patch queued for re-testing.

Crell’s picture

Crell commented
Status: Needs review » Needs work 
+++ b/core/includes/menu.inc
@@ -917,7 +926,8 @@ function _menu_link_translate(&$item, $translate = FALSE) {
+        $item['access'] = drupal_container()->get('access_manager')->check($route, $request);

This should be Drupal::service('access_manager')

There's also a ton of unrelated config system stuff going on in there. Looks like a patch got mixed up by accident.

kim.pepper’s picture

kim.pepper
Primary language English
Location 🏄‍♂️🇦🇺Sydney, Australia
commented
Status: Needs work » Needs review
StatusFileSize
new 1998708-block-request-8.patch2.42 KB

Looks like a bad rebase. Trying again...

Crell’s picture

Crell commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Quick fix

That's more like it. :-)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 76518e6 and pushed to 8.x. Thanks!

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