Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

CommentFileSizeAuthor
#66 batch-1987816-66.patch7.89 KBdawehner
#63 drupal8.system-module.1987816-63.patch7.89 KBdisasm
#63 interdiff.txt413 bytesdisasm
#62 drupal8.system_batch_page.1987816-62.patch7.84 KBdisasm
#62 interdiff.txt395 bytesdisasm
#58 drupal8.system_batch_page.1987816-58.patch7.86 KBdisasm
#52 drupal8.system_batch_page.1987816-52.patch150.55 KBdisasm
#52 interdiff.txt413 bytesdisasm
#48 drupal8.system-module.1987816-48.patch3.51 KBdisasm
#48 interdiff.txt5.03 KBdisasm
#44 drupal8.system-module.1987816-44.patch7.91 KBdisasm
#39 1987816-system_batch_page-controller-39.patch7.92 KBvijaycs85
#39 1987816-diff-37-39.txt572 bytesvijaycs85
#37 1987816-system_batch_page-controller-37.patch7.93 KBvijaycs85
#37 1987816-diff-31-37.txt802 bytesvijaycs85
#31 drupal-batch_page_controller-1987816-31.patch7.95 KBpguillard
#28 drupal-batch_page_controller-1987816-28.patch7.84 KBpguillard
#27 drupal-batch_page_controller-1987816-27.patch7.84 KBpguillard
#24 interdiff.txt653 bytesParisLiakos
#23 drupal-batch_page_controller-1987816-22.patch7.72 KBParisLiakos
#22 drupal-batch_page_controller-1987816-20.patch7.71 KBParisLiakos
#20 drupal-batch_page_controller-1987816-20.patch7.71 KBParisLiakos
#15 drupal-batch_page_controller-1987816-15.patch8.05 KBpdrake
#15 interdiff.txt594 bytespdrake
#13 drupal-batch_page_controller-1987816-13.patch8.05 KBpdrake
#13 interdiff.txt896 bytespdrake
#9 drupal-batch_page_controller-1987816-9.patch7.71 KBpdrake
#8 drupal-batch_page_controller-1987816-8.patch0 bytespdrake
#7 drupal-batch_page_controller-1987816-6.patch7.25 KBpdrake
#5 drupal-batch_page_controller-1987816-5.patch7.35 KBpdrake
#3 drupal-batch_page_controller-1987816-3.patch7.58 KBpdrake
#1 drupal-batch_page_controller-1987816-1.patch6.88 KBpdrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

pdrake’s picture

Assigned: Unassigned » pdrake
Status: Active » Needs review
FileSize
6.88 KB

Status: Needs review » Needs work

The last submitted patch, drupal-batch_page_controller-1987816-1.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review
FileSize
7.58 KB
dawehner’s picture

+++ b/core/authorize.phpundefined
@@ -68,6 +70,13 @@ function authorize_access_allowed() {
+drupal_container()
+  ->set('request', $request);

Yeah there is a @todo but anyway: I guess this should be Drupal::setContainer

+++ b/core/includes/install.core.incundefined
@@ -604,7 +604,7 @@ function install_run_task($task, &$install_state) {
+      $output = _batch_page(drupal_container()->get('request'));

This could use Drupal::request()

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -0,0 +1,69 @@
+  /**
+   * Request object.
+   *
+   * @var \Symfony\Component\HttpFoundation\Request
+   */
+  protected $request;
...
+  /**
+   * Constructs a SystemController object.
+   */
+  public function __construct(Request $request) {
+    $this->request = $request;
+  }

Instead of injecting the request object you can directly have this on the controller method, so it looks like that: public function batchPage(Request $request) { ... }

+++ b/core/modules/system/system.moduleundefined
+++ b/core/modules/system/system.moduleundefined
@@ -1013,11 +1013,9 @@ function system_menu() {

@@ -1013,11 +1013,9 @@ function system_menu() {
 
   // Default page for batch operations.
   $items['batch'] = array(
-    'page callback' => 'system_batch_page',
-    'access callback' => TRUE,
+    'route_name' => 'system_batch_page',
     'theme callback' => '_system_batch_theme',
     'type' => MENU_CALLBACK,
-    'file' => 'system.admin.inc',
   );

Callbacks can be skipped totally from hook_menu.

pdrake’s picture

I think this patch addresses everything except your comment in authorize.php. That was copied from elsewhere, might make sense to just address the todo in all places at once (which could be in this patch, or not).

ParisLiakos’s picture

i really like this patch! very good job, thanks.
some notes:

+++ b/core/authorize.phpundefined
@@ -68,6 +70,13 @@ function authorize_access_allowed() {
+// @todo These two lines were copied from index.php which has its own todo about
+// a change required here. Revisit this when that change has been made.
...
+drupal_container()
+  ->set('request', $request);

i doubt this todo will be removed in D8:(
but drupal_container will.
so better use
Drupal::getContainer()->set(..);

+++ b/core/includes/install.core.incundefined
@@ -604,7 +604,7 @@ function install_run_task($task, &$install_state) {
+      $output = _batch_page(drupal_container()->get('request'));

should be Drupal::request()

+++ b/core/modules/system/lib/Drupal/system/Controller/BatchController.phpundefined
@@ -0,0 +1,47 @@
+   * @return string
+   *   A HTML-formatted string with the batch page content.

this also needs updating, since we are returning a Response

+++ b/core/modules/system/lib/Drupal/system/Controller/BatchController.phpundefined
@@ -0,0 +1,47 @@
+      return $page;

this should return a Response object i think

pdrake’s picture

This version of the patch restores the menu callback because of the theme callback.

pdrake’s picture

Ok, here's another try, addressing most of the comments in #6.

pdrake’s picture

That didn't work. Trying again.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

The last submitted patch, drupal-batch_page_controller-1987816-9.patch, failed testing.

pdrake’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion
ParisLiakos’s picture

looks great! just a nitpick

+++ b/core/modules/system/lib/Drupal/system/Controller/BatchController.phpundefined
@@ -0,0 +1,47 @@
+   *   A Response object or page element.

we use the full namespace on docblocks

+++ b/core/update.phpundefined
@@ -414,11 +414,8 @@ function update_check_requirements($skip_warnings = FALSE) {
-// @todo These two lines were copied from index.php which has its own todo about
-// a change required here. Revisit this when that change has been made.

i am not sure whether we should remove this, but i agree with removing it, since fixing this would require quite some work, which is D9 material imo
Also, the @todo that this refers to, no longer exists...so maybe just rephrase it to:

@todo Refactor this.

if not removing completely

pdrake’s picture

ParisLiakos’s picture

sorry for that, but full namespace needs the leading \

pdrake’s picture

dawehner’s picture

It would be cool to ensure that the theme callback is still working, but i'm not really sure how to ensure that.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

i think its ready to go

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +WSCCI-conversion

The last submitted patch, drupal-batch_page_controller-1987816-15.patch, failed testing.

ParisLiakos’s picture

Assigned: pdrake » Unassigned
Status: Needs work » Needs review
FileSize
7.71 KB

reroll

Status: Needs review » Needs work

The last submitted patch, drupal-batch_page_controller-1987816-20.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
7.71 KB

yeah, no has there

ParisLiakos’s picture

same patch..sigh

ParisLiakos’s picture

FileSize
653 bytes

interdiff

dawehner’s picture

+++ b/core/includes/batch.incundefined
@@ -14,23 +14,27 @@
-  if (!isset($_REQUEST['id'])) {
...
-    $batch = Drupal::service('batch.storage')->load($_REQUEST['id']);

@@ -49,7 +53,7 @@ function _batch_page() {
-  $op = isset($_REQUEST['op']) ? $_REQUEST['op'] : '';
+  $op = $request->get('op', '');

I'm wondering whether it might be possible to know whether this is $_GET or $_POST all the time? In that case we could directly access the values via $request->query->get for example.

ParisLiakos’s picture

i agree, i think its always GET, but i am not sure, thats why i left it that way

pguillard’s picture

- I re-rolled that patch and replaced $request->get('op', ''); with $request->query->get('op', '');
- Tested quickly by checking manually for updates here /admin/reports/updates/check

Hope it's fine, but please check, as I'm not experienced enough !!

pguillard’s picture

ParisLiakos, corrected the missing $request->query->get

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks!

+++ b/core/modules/system/lib/Drupal/system/Controller/BatchController.phpundefined
@@ -0,0 +1,46 @@
+   *   A \Symfony\Component\HttpFoundation\Response object or page element.
+   *
+   */

this middle line is not needed

+++ b/core/modules/system/lib/Drupal/system/Controller/BatchController.phpundefined
@@ -0,0 +1,46 @@
+  public function batchPage(Request $request) {

Seems we forgot to add @param documentation for the $request parameter

ParisLiakos’s picture

Status: Reviewed & tested by the community » Needs work

eer..i meant to NW

pguillard’s picture

Status: Needs work » Needs review
FileSize
7.95 KB

Done

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

perfect, thanks!

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

fubhy’s picture

Wait, what's with RouteBuilderStatic? We added a @todo for this issue there but we don't touch it with the patch from #31.

ParisLiakos’s picture

i dont have a clue..why would we need that in this conversion? or maybe its not needed now? dunno really, but we can open a followup to investigate it.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/authorize.phpundefined
@@ -67,6 +69,10 @@ function authorize_access_allowed() {
+// A request object from the HTTPFoundation to tell us about the request.

This comment is unnecessary.

+++ b/core/authorize.phpundefined
@@ -133,7 +139,7 @@ function authorize_access_allowed() {
   elseif (isset($_GET['batch'])) {

We now should be using the request here... instead of getting from global state.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +LONDON_2013_JULY
FileSize
802 bytes
7.93 KB

Updating changes for #36

dawehner’s picture

+++ b/core/includes/batch.incundefined
@@ -16,23 +16,27 @@
-  if (!isset($_REQUEST['id'])) {
+  if (!($request_id = $request->query->get('id'))) {

@@ -51,7 +55,7 @@ function _batch_page() {
-  $op = isset($_REQUEST['op']) ? $_REQUEST['op'] : '';
+  $op = $request->query->get('op', '');

$_REQUEST also potentially contain $_POST information, so let's go with $request->get('id'), as it is safer, see
the comments #25, #26# and #27 on this issue.

vijaycs85’s picture

Sure, updating as per #38...

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you very much!

xjm’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/update.phpundefined
@@ -430,6 +430,8 @@ function update_check_requirements($skip_warnings = FALSE) {
+// @todo Refactor this.

This @todo is a bit vague. It seems this was suggested in #12 and introduced in #13. Actually, we should remove the @todo as the request service is synthetic and this is exactly what #2004086: The Request service must be synthetic is adding.

dawehner’s picture

Ignore that please.

disasm’s picture

Status: Needs work » Needs review
FileSize
7.91 KB

reroll!

jibran’s picture

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion

NW because of #42.

jibran’s picture

Issue tags: +WSCCI-conversion

NW because of #42.

dawehner’s picture

  1. +++ b/core/authorize.php
    @@ -20,6 +20,8 @@
    +use Symfony\Component\HttpFoundation\Request;
    +
    
    @@ -69,6 +71,9 @@ function authorize_access_allowed() {
     
    +$request = Request::createFromGlobals();
    +Drupal::getContainer()->set('request', $request);
    +
    

    I am wondering why you did this change. On the longrun this will be done using synthetic on the request service but is this really needed here?

  2. +++ b/core/modules/system/system.routing.yml
    @@ -234,3 +234,10 @@ system_admin_config:
    +    _controller: '\Drupal\system\Controller\BatchController::batchPage'
    

    If you do return HTML in the sense of a drupal page you should use rather _content than _controller

disasm’s picture

Status: Needs work » Needs review
FileSize
5.03 KB
3.51 KB

Lets see if this passes without all the additional changes to core files. I like having the $request object instead of $_REQUEST in _batch_page() but this is touching way too many things in my opinion and I think that's out of scope for this conversion.

dawehner’s picture

+++ w/core/authorize.php
@@ -141,8 +136,8 @@ function authorize_access_allowed() {
   }
   // If a batch is running, let it run.
-  elseif ($request->query->has('batch')) {
-    $output = _batch_page($request);
+  elseif (isset($_GET['batch'])) {
+    $output = _batch_page();
   }
   else {

+++ w/core/includes/batch.inc
@@ -16,27 +16,23 @@
 
-  if (!($request_id = $request->get('id'))) {
+  if (!isset($_REQUEST['id'])) {
     return FALSE;
...
   if (!$batch) {
-    $batch = Drupal::service('batch.storage')->load($request_id);
+    $batch = Drupal::service('batch.storage')->load($_REQUEST['id']);
     if (!$batch) {
       drupal_set_message(t('No active batch.'), 'error');
       return new RedirectResponse(url('<front>', array('absolute' => TRUE)));

@@ -55,7 +51,7 @@ function _batch_page(Request $request) {
-  $op = $request->get('op', '');
+  $op = isset($_REQUEST['op']) ? $_REQUEST['op'] : '';
   $output = NULL;
   switch ($op) {

+++ w/core/includes/install.core.inc
@@ -652,7 +652,7 @@ function install_run_task($task, &$install_state) {
-      $output = _batch_page(Drupal::request());
+      $output = _batch_page();

+++ w/core/modules/system/lib/Drupal/system/Controller/BatchController.php
@@ -27,7 +27,7 @@ class BatchController {
     require_once DRUPAL_ROOT . '/core/includes/batch.inc';
-    $output = _batch_page($request);
+    $output = _batch_page();
 
     if ($output === FALSE) {

+++ w/core/update.php
@@ -554,7 +555,7 @@ function update_check_requirements($skip_warnings = FALSE) {
       update_task_list('run');
-      $output = _batch_page($request);
+      $output = _batch_page();
       break;
   }

These changes are perfectly fine

xjm’s picture

Thanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.

dawehner’s picture

Status: Needs review » Needs work
disasm’s picture

Status: Needs work » Needs review
FileSize
413 bytes
150.55 KB

Undoing #48. Removing setting the request int the container. We'll see if this passes that way or if it's required. As for changing to _content, the method has detailed comments about how we don't want to create a full page so the message will be displayed on the following page load, hence the usage of _controller.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion, -RTBC July 1, -LONDON_2013_JULY

The last submitted patch, drupal8.system_batch_page.1987816-52.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal8.system_batch_page.1987816-52.patch, failed testing.

disasm’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion, +RTBC July 1, +LONDON_2013_JULY
dawehner’s picture

Status: Needs review » Needs work

The size of the patch is too damn high.

disasm’s picture

Status: Needs work » Needs review
FileSize
7.86 KB

try this one.

dawehner’s picture

I kind of think that we need manual testing on this one as batch is damn complicated: For example the manual testing has to ensure that the admin theme is still used.

ParisLiakos’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing
+++ w/core/update.php
@@ -432,6 +432,8 @@ function update_check_requirements($skip_warnings = FALSE) {
+// @todo Refactor this.
+

lets remove this @todo like #42 points out

Also, lets please restore the set('request', $request) call in authorize.php which was removed in #52
Since we are creating the request, lets put in the container, so we avoid potential breakage in future of services that depend on it

disasm’s picture

I ran a few simple tests as well as enabled content translation for pages. Are there any specific other manual tests that need done? I experienced no errors in my manual testing.

disasm’s picture

Status: Needs work » Needs review
FileSize
395 bytes
7.84 KB

adds back the thing that was removed that I think was added at some point. You get the picture.

disasm’s picture

adds back setting of request in container in authorize.php.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

lets get this finally in..if bot agrees ofc

webchick’s picture

Status: Reviewed & tested by the community » Needs work
+++ w/core/authorize.php
@@ -71,6 +73,9 @@ function authorize_access_allowed() {
+$request = Request::createFromGlobals();
+Drupal::getContainer()->set('request', $request);
+

This part looked weird to me, but I notice we do the same thing in update.php so apparently this is just stuff you have to do in standalone scripts these days.

However, no longer applies. :(

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.89 KB

This bit is also done in #2004086: The Request service must be synthetic, totally fine.

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Tested manually worked fine. Back to RTBC.

jibran’s picture

Issue tags: -Needs manual testing

Removing the tag.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

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