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
#18 1987850-route-system-theme-disable-13.patch8.32 KBoenie
PASSED: [[SimpleTest]]: [MySQL] 57,397 pass(es).
[ View ]
#15 1987850-route-system-theme-disable-12.patch8.32 KBoenie
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#15 diff-1987850-route-system-theme-disable-11.txt3.84 KBoenie
#14 1987850-route-system-theme-disable-12.patch8.32 KBoenie
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987850-route-system-theme-disable-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#14 diff-1987850-route-system-theme-disable-11.patch3.84 KBoenie
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch diff-1987850-route-system-theme-disable-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#12 1987850-route-system-theme-disable-11.patch8.43 KBamateescu
PASSED: [[SimpleTest]]: [MySQL] 57,195 pass(es).
[ View ]
#12 interdiff.txt2.71 KBamateescu
#11 1987850_8.patch5.26 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#9 1987850_7.patch5.26 KBcosmicdreams
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#7 1987850-route-system-theme-disable-3.patch8.41 KBoenie
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987850-route-system-theme-disable-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#2 1987850-route-system-theme-disable-2.patch4.48 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#2 1987850-diff-1-2.txt1.82 KBvijaycs85
#1 1987850-route-system-theme-disable-1.patch2.66 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 55,669 pass(es).
[ View ]

Comments

vijaycs85’s picture

Status:Active» Needs review
StatusFileSize
new2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 55,669 pass(es).
[ View ]

Initial patch...

vijaycs85’s picture

StatusFileSize
new1.82 KB
new4.48 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Adding the controller class that is missing in #1

amateescu’s picture

Status:Needs review» Needs work
+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
@@ -0,0 +1,46 @@
+use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
+/**
+ * Controller for theme handling.

Missing an empty line here.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
@@ -0,0 +1,46 @@
+        if ($theme === config('system.theme')->get('default') || $theme === config('system.theme')->get('admin')) {

Config should be injected.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
@@ -0,0 +1,46 @@
+      drupal_goto('admin/appearance');

This should use RedirectResponse.

oenie’s picture

Assigned:Unassigned» oenie

I'm taking up this issue and joining it with issue #1987852: Convert system_theme_enable() to a new style controller

oenie’s picture

amateescu,

You mention in comment #3 that the config should be injected.
I'm trying to find an example for this, but can't seem to find anything useful.

I tried injecting a ConfigFactory, but then i end up with the next error:

Drupal\Core\Config\ConfigImporterException: config.installer is already importing in Drupal\Core\Config\ConfigImporter->import() (line 219 of /Users/Jeroen/development/drupal/core/lib/Drupal/Core/Config/ConfigImporter.php)

Do you have any pointers or examples of where to look ?

Thanks !

amateescu’s picture

Sure, you can use this one as an example: StatisticsSettingsForm::create().

If that doesn't work out, could you post your patch anyway so more people can help you debug it? It's also possible that this might be a bug coming from this newly committed issue #1890784: Refactor configuration import and sync functions.

oenie’s picture

Status:Needs work» Needs review
StatusFileSize
new8.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987850-route-system-theme-disable-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's my current patch, which to me seems rather complete, but ends up with the error mentioned in comment #5.
Maybe i'm missing something ?

Status:Needs review» Needs work

The last submitted patch, 1987850-route-system-theme-disable-3.patch, failed testing.

cosmicdreams’s picture

StatusFileSize
new5.26 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Here's a patch that implements a proper constructor and implements the ControllerInterface. As a result, I'm injecting the config factory and using in the disable() function.

kim.pepper’s picture

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -0,0 +1,74 @@
+   * @var \Drupal\Core\Config\ConfigFactory|ConfigFactory

Needs comment

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -0,0 +1,74 @@
+      $config = $this->configFactory;

Should $config = $this->configFactory->get('system.theme');

cosmicdreams’s picture

StatusFileSize
new5.26 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

In #7, I"m not getting the right set of config. This patch gets system.theme config properly.

amateescu’s picture

Status:Needs work» Needs review
StatusFileSize
new2.71 KB
new8.43 KB
PASSED: [[SimpleTest]]: [MySQL] 57,195 pass(es).
[ View ]

@cosmicdreams, this issue was assigned to @oenie and he is working on it..

@oenie, I applied the patch locally (after fixing the conflict in system.routing.yml) and theme enabling/disabling works just fine, no errrors at all :) Let's try to feed the testbot an updated patch.

PS. I also fixed some trailing whitespaces and code style errors. Interdiff is to #7.

dawehner’s picture

Status:Needs review» Needs work
+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -0,0 +1,126 @@
+        if ($theme === $this->configFactory->get('system.theme')->get('default') || $theme === $this->configFactory->get('system.theme')->get('admin')) {

Let's just inject the system.theme config object and not the full factory

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -0,0 +1,126 @@
+      return new RedirectResponse(url("admin/appearance", array('absolute' => TRUE)));
...
+      return new RedirectResponse(url("admin/appearance", array('absolute' => TRUE)));

/me hides ... single quotes.

+++ b/core/modules/system/system.moduleundefined
@@ -704,17 +704,13 @@ function system_menu() {
   $items['admin/appearance/enable'] = array(
     'title' => 'Enable theme',
-    'page callback' => 'system_theme_enable',
-    'access arguments' => array('administer themes'),
+    'route_name' => 'system_theme_enable',
     'type' => MENU_CALLBACK,
-    'file' => 'system.admin.inc',
   );
   $items['admin/appearance/disable'] = array(
     'title' => 'Disable theme',
-    'page callback' => 'system_theme_disable',
-    'access arguments' => array('administer themes'),
+    'route_name' => 'system_theme_disable',
     'type' => MENU_CALLBACK,
-    'file' => 'system.admin.inc',

Let's get rid of it ... they are menu callbacks.

oenie’s picture

StatusFileSize
new3.84 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch diff-1987850-route-system-theme-disable-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new8.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987850-route-system-theme-disable-12.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Another try, including dawehner's comments.
I've attached an interdiff to the patch of comment 11

oenie’s picture

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

Let's try that again as a patch.
Another try, including dawehner's comments.
I've attached an interdiff to the patch of comment #12 by amateescu

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

The last submitted patch, 1987850-route-system-theme-disable-12.patch, failed testing.

oenie’s picture

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

StatusFileSize
new8.32 KB
PASSED: [[SimpleTest]]: [MySQL] 57,397 pass(es).
[ View ]

Reroll of the patch against the current code.
Learning a bunch of new stuff :)

cosmicdreams’s picture

Status:Needs review» Reviewed & tested by the community

Looks great!

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

Committed f94711c and pushed to 8.x. Thanks!

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