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
#23 interdiff.txt1.23 KBh3rj4n
#23 system-1987808-23.patch3.81 KBh3rj4n
PASSED: [[SimpleTest]]: [MySQL] 57,989 pass(es).
[ View ]
#21 interdiff.txt968 bytesh3rj4n
#21 system-1987808-20.patch3.52 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 57,929 pass(es), 6 fail(s), and 6 exception(s).
[ View ]
#15 interdiff.txt722 bytesh3rj4n
#15 system-1987808-15.patch6.09 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-1987808-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#13 system-1987808-13.patch6.08 KBtim.plunkett
PASSED: [[SimpleTest]]: [MySQL] 56,808 pass(es).
[ View ]
#10 1987808-page-controller-admin_compact-10.patch3.66 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 56,040 pass(es).
[ View ]
#10 1987808-diff-4-10.txt1.08 KBvijaycs85
#4 admin_compact-1987808-4.patch3.58 KBDave.Ingram
PASSED: [[SimpleTest]]: [MySQL] 55,806 pass(es).
[ View ]
#1 1987808-page-controller-admin_compact-1.patch3.32 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 56,209 pass(es), 6 fail(s), and 6 exception(s).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new3.32 KB
FAILED: [[SimpleTest]]: [MySQL] 56,209 pass(es), 6 fail(s), and 6 exception(s).
[ View ]

Initial patch...

Status:Needs review» Needs work

The last submitted patch, 1987808-page-controller-admin_compact-1.patch, failed testing.

Assigned:Unassigned» Dave.Ingram

I'll have a look at these errors.

Status:Needs work» Needs review
StatusFileSize
new3.58 KB
PASSED: [[SimpleTest]]: [MySQL] 55,806 pass(es).
[ View ]

Updated this patch to fix the errors. Tested it manually and all seems to be working.

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

The last submitted patch, admin_compact-1987808-4.patch, failed testing.

Status:Needs work» Needs review

#4: admin_compact-1987808-4.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, admin_compact-1987808-4.patch, failed testing.

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

#4: admin_compact-1987808-4.patch queued for re-testing.

+++ b/core/modules/system/lib/Drupal/system/Controller/AdminPageController.phpundefined
@@ -0,0 +1,28 @@
+   * @param $mode

Should be @param string $node

+++ b/core/modules/system/lib/Drupal/system/Controller/AdminPageController.phpundefined
@@ -0,0 +1,28 @@
+    // @todo refactor once drupal.org/node/1668866 is in.
+    drupal_goto();

I can't see why we can't use RedirectResponse already.

StatusFileSize
new1.08 KB
new3.66 KB
PASSED: [[SimpleTest]]: [MySQL] 56,040 pass(es).
[ View ]

updating changes mentioned in #9.

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Controller/AdminPageController.php
@@ -0,0 +1,30 @@
+    return new RedirectResponse(url('', array('absolute' => TRUE)));

This won't work correctly if Drupal is in a subdirectory. We need instead to inject the generator, and/or rely on code in #1668866: Replace drupal_goto() with RedirectResponse and #1623114: Remove drupal_exit() . That's unfortunately a bit complicated, especially with the generator issue only RTBC, not committed.

We discussed this at the sprint on Friday, and decided that for now we can leave the drupal_goto() in place with a @todo.

+++ b/core/modules/system/system.routing.yml
@@ -115,3 +116,11 @@ system_theme_enable:
+system_admin_compact_page:
+  pattern: 'admin/compact/{mode}'
+  defaults:
+    _controller: 'Drupal\system\Controller\AdminPageController::compactPage'
+    mode: 'off'
+  requirements:

Something else just occurred to me. We ARE posting to this route, right? not GETing? If so, we can probably restrict it to just POST.

If we are GETing it, bad on us and we should see if that can be fixed.

+++ b/core/modules/system/system.moduleundefined
@@ -657,11 +657,8 @@ function system_menu() {
   $items['admin/compact'] = array(
-    'title' => 'Compact mode',
-    'page callback' => 'system_admin_compact_page',
-    'access arguments' => array('access administration pages'),
+    'route_name' => 'system_admin_compact_page',
     'type' => MENU_CALLBACK,
-    'file' => 'system.admin.inc',

this full entry can now also be removed, as it is not longer needed.

Status:Needs work» Needs review
StatusFileSize
new6.08 KB
PASSED: [[SimpleTest]]: [MySQL] 56,808 pass(es).
[ View ]

Rerolled, injected, refactored.

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -53,4 +104,17 @@ public function autocomplete($plugin_id, Request $request) {
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse
+   */
+  function compactPage($mode) {

Needs some docs and public

Assigned:Dave.Ingram» Unassigned
Status:Needs work» Needs review
StatusFileSize
new6.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-1987808-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new722 bytes

Added the public part. What do you mean with docs? Some more documentation? Where?

Status:Needs review» Needs work

The last submitted patch, system-1987808-15.patch, failed testing.

I don't know why I put docs in there. I think it just needed the public keyword.

Because it needs something on the line after @return.

It couldn't be applied because of the commit of #2056513: Remove PluginUI subsystem, provide block plugins UI as one-off. That happened just before I updated the issue I guess.

Can I just move the code as documented in that issue?

SystemController to BlockAutocompleteController (routing controller)

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
@@ -53,4 +104,17 @@ public function autocomplete($plugin_id, Request $request) {
+  /**
+   * Sets whether the admin menu is in compact mode or not.
+   *
+   * @param string $mode
+   *   Valid values are 'on' and 'off'.
+   *
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse
+   */
+  function compactPage($mode) {
+    user_cookie_save(array('admin_compact_mode' => ($mode == 'on')));
+    return new RedirectResponse(url('<front>', array('absolute' => TRUE)));
+  }

I'll add this to the other class.

No, you need to reintroduce SystemController with just your method.

Status:Needs work» Needs review
StatusFileSize
new3.52 KB
FAILED: [[SimpleTest]]: [MySQL] 57,929 pass(es), 6 fail(s), and 6 exception(s).
[ View ]
new968 bytes

Added the changes.

Status:Needs review» Needs work

The last submitted patch, system-1987808-20.patch, failed testing.

StatusFileSize
new3.81 KB
PASSED: [[SimpleTest]]: [MySQL] 57,989 pass(es).
[ View ]
new1.23 KB

I just missed your comment but your absolutely right. I wasn't reading. Changed it so that it works. The tests don't fail any more on my system.

Status:Needs work» Needs review

Status:Needs review» Reviewed & tested by the community

Let's just get this over with. :-) Thanks, h3rj4n.

@@ -0,0 +1,40 @@
+    return new RedirectResponse(url('<front>', array('absolute' => TRUE)));

Well instead of url() we could also directly use generateFromPath on the url generator.

I'm assuming we'll change that line again once #2052995: Add a route to the frontpage is in, but no sense waiting for that.

I agree.

Status:Reviewed & tested by the community» Fixed

Committed f325f63 and pushed to 8.x. Thanks!

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
@@ -0,0 +1,40 @@
+class SystemController implements ControllerInterface {
...
+  /**
+   * {@inheritdoc}
+   */
+  public static function create(ContainerInterface $container) {
+    return new static();
+  }

Can someone explain why this part was added? Is this some sort of new standard that controllers shoud implement ControllerInterface?

It resulted from blindly following the conversion instructions. It's not harmful, and may eventually be used, but is overkill at this point.
However, the committees have indicated they're okay with this.

OK, sure, I can live with it. Thanks!

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