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.

Files: 
CommentFileSizeAuthor
#66 batch-1987816-66.patch7.89 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 59,114 pass(es).
[ View ]
#63 drupal8.system-module.1987816-63.patch7.89 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,964 pass(es).
[ View ]
#63 interdiff.txt413 bytesdisasm
#62 drupal8.system_batch_page.1987816-62.patch7.84 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#62 interdiff.txt395 bytesdisasm
#58 drupal8.system_batch_page.1987816-58.patch7.86 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 59,263 pass(es).
[ View ]
#52 drupal8.system_batch_page.1987816-52.patch150.55 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,663 pass(es).
[ View ]
#52 interdiff.txt413 bytesdisasm
#48 drupal8.system-module.1987816-48.patch3.51 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,950 pass(es).
[ View ]
#48 interdiff.txt5.03 KBdisasm
#44 drupal8.system-module.1987816-44.patch7.91 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,420 pass(es).
[ View ]
#39 1987816-system_batch_page-controller-39.patch7.92 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,504 pass(es).
[ View ]
#39 1987816-diff-37-39.txt572 bytesvijaycs85
#37 1987816-system_batch_page-controller-37.patch7.93 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,573 pass(es).
[ View ]
#37 1987816-diff-31-37.txt802 bytesvijaycs85
#31 drupal-batch_page_controller-1987816-31.patch7.95 KBpguillard
PASSED: [[SimpleTest]]: [MySQL] 57,966 pass(es).
[ View ]
#28 drupal-batch_page_controller-1987816-28.patch7.84 KBpguillard
PASSED: [[SimpleTest]]: [MySQL] 56,677 pass(es).
[ View ]
#27 drupal-batch_page_controller-1987816-27.patch7.84 KBpguillard
PASSED: [[SimpleTest]]: [MySQL] 58,143 pass(es).
[ View ]
#24 interdiff.txt653 bytesParisLiakos
#23 drupal-batch_page_controller-1987816-22.patch7.72 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 57,966 pass(es).
[ View ]
#22 drupal-batch_page_controller-1987816-20.patch7.71 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#20 drupal-batch_page_controller-1987816-20.patch7.71 KBParisLiakos
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#15 drupal-batch_page_controller-1987816-15.patch8.05 KBpdrake
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-batch_page_controller-1987816-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 interdiff.txt594 bytespdrake
#13 drupal-batch_page_controller-1987816-13.patch8.05 KBpdrake
PASSED: [[SimpleTest]]: [MySQL] 55,854 pass(es).
[ View ]
#13 interdiff.txt896 bytespdrake
#9 drupal-batch_page_controller-1987816-9.patch7.71 KBpdrake
PASSED: [[SimpleTest]]: [MySQL] 56,807 pass(es).
[ View ]
#8 drupal-batch_page_controller-1987816-8.patch0 bytespdrake
PASSED: [[SimpleTest]]: [MySQL] 56,990 pass(es).
[ View ]
#7 drupal-batch_page_controller-1987816-6.patch7.25 KBpdrake
PASSED: [[SimpleTest]]: [MySQL] 55,824 pass(es).
[ View ]
#5 drupal-batch_page_controller-1987816-5.patch7.35 KBpdrake
PASSED: [[SimpleTest]]: [MySQL] 57,057 pass(es).
[ View ]
#3 drupal-batch_page_controller-1987816-3.patch7.58 KBpdrake
PASSED: [[SimpleTest]]: [MySQL] 56,940 pass(es).
[ View ]
#1 drupal-batch_page_controller-1987816-1.patch6.88 KBpdrake
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Assigned:Unassigned» pdrake
Status:Active» Needs review
StatusFileSize
new6.88 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new7.58 KB
PASSED: [[SimpleTest]]: [MySQL] 56,940 pass(es).
[ View ]

+++ 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.

StatusFileSize
new7.35 KB
PASSED: [[SimpleTest]]: [MySQL] 57,057 pass(es).
[ View ]

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).

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

StatusFileSize
new7.25 KB
PASSED: [[SimpleTest]]: [MySQL] 55,824 pass(es).
[ View ]

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

StatusFileSize
new0 bytes
PASSED: [[SimpleTest]]: [MySQL] 56,990 pass(es).
[ View ]

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

StatusFileSize
new7.71 KB
PASSED: [[SimpleTest]]: [MySQL] 56,807 pass(es).
[ View ]

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.

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

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

StatusFileSize
new896 bytes
new8.05 KB
PASSED: [[SimpleTest]]: [MySQL] 55,854 pass(es).
[ View ]

sorry for that, but full namespace needs the leading \

StatusFileSize
new594 bytes
new8.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-batch_page_controller-1987816-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

Status:Needs review» Reviewed & tested by the community

i think its ready to go

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.

Assigned:pdrake» Unassigned
Status:Needs work» Needs review
StatusFileSize
new7.71 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

reroll

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new7.71 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

yeah, no has there

StatusFileSize
new7.72 KB
PASSED: [[SimpleTest]]: [MySQL] 57,966 pass(es).
[ View ]

same patch..sigh

StatusFileSize
new653 bytes

interdiff

+++ 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.

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

StatusFileSize
new7.84 KB
PASSED: [[SimpleTest]]: [MySQL] 58,143 pass(es).
[ View ]

- 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 !!

StatusFileSize
new7.84 KB
PASSED: [[SimpleTest]]: [MySQL] 56,677 pass(es).
[ View ]

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

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

Status:Reviewed & tested by the community» Needs work

eer..i meant to NW

Status:Needs work» Needs review
StatusFileSize
new7.95 KB
PASSED: [[SimpleTest]]: [MySQL] 57,966 pass(es).
[ View ]

Done

Status:Needs review» Reviewed & tested by the community

perfect, thanks!

Issue tags:+RTBC July 1

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

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

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.

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.

Status:Needs work» Needs review
Issue tags:+LONDON_2013_JULY
StatusFileSize
new802 bytes
new7.93 KB
PASSED: [[SimpleTest]]: [MySQL] 57,573 pass(es).
[ View ]

Updating changes for #36

+++ 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.

StatusFileSize
new572 bytes
new7.92 KB
PASSED: [[SimpleTest]]: [MySQL] 57,504 pass(es).
[ View ]

Sure, updating as per #38...

Status:Needs review» Reviewed & tested by the community

Thank you very much!

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.

Ignore that please.

Status:Needs work» Needs review
StatusFileSize
new7.91 KB
PASSED: [[SimpleTest]]: [MySQL] 58,420 pass(es).
[ View ]

reroll!

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

NW because of #42.

Issue tags:+WSCCI-conversion

NW because of #42.

  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

Status:Needs work» Needs review
StatusFileSize
new5.03 KB
new3.51 KB
PASSED: [[SimpleTest]]: [MySQL] 58,950 pass(es).
[ View ]

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.

+++ 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

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.

Status:Needs review» Needs work

Status:Needs work» Needs review
StatusFileSize
new413 bytes
new150.55 KB
PASSED: [[SimpleTest]]: [MySQL] 58,663 pass(es).
[ View ]

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.

Status:Needs work» Needs review

Status:Needs review» Needs work

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

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

Status:Needs review» Needs work

The size of the patch is too damn high.

Status:Needs work» Needs review
StatusFileSize
new7.86 KB
PASSED: [[SimpleTest]]: [MySQL] 59,263 pass(es).
[ View ]

try this one.

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.

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

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.

Status:Needs work» Needs review
StatusFileSize
new395 bytes
new7.84 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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

StatusFileSize
new413 bytes
new7.89 KB
PASSED: [[SimpleTest]]: [MySQL] 58,964 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

lets get this finally in..if bot agrees ofc

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. :(

Status:Needs work» Needs review
StatusFileSize
new7.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,114 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Tested manually worked fine. Back to RTBC.

Issue tags:-Needs manual testing

Removing the tag.

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

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