Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
3.32 KB

Initial patch...

Status: Needs review » Needs work

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

Dave.Ingram’s picture

Assigned: Unassigned » Dave.Ingram

I'll have a look at these errors.

Dave.Ingram’s picture

Status: Needs work » Needs review
FileSize
3.58 KB

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.

Dave.Ingram’s picture

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.

Dave.Ingram’s picture

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

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

dawehner’s picture

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

vijaycs85’s picture

updating changes mentioned in #9.

Crell’s picture

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.

dawehner’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
6.08 KB

Rerolled, injected, refactored.

dawehner’s picture

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

h3rj4n’s picture

Assigned: Dave.Ingram » Unassigned
Status: Needs work » Needs review
FileSize
6.09 KB
722 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.

Crell’s picture

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

tim.plunkett’s picture

Because it needs something on the line after @return.

h3rj4n’s picture

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.

tim.plunkett’s picture

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

h3rj4n’s picture

Status: Needs work » Needs review
FileSize
3.52 KB
968 bytes

Added the changes.

Status: Needs review » Needs work

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

h3rj4n’s picture

FileSize
3.81 KB
1.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.

h3rj4n’s picture

Status: Needs work » Needs review
Crell’s picture

Status: Needs review » Reviewed & tested by the community

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

dawehner’s picture

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

Crell’s picture

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.

dawehner’s picture

I agree.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f325f63 and pushed to 8.x. Thanks!

tstoeckler’s picture

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

tim.plunkett’s picture

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.

tstoeckler’s picture

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

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