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
FileSize
2.66 KB
PASSED: [[SimpleTest]]: [MySQL] 55,669 pass(es). View

Initial patch...

vijaycs85’s picture

FileSize
1.82 KB
4.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
FileSize
8.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

FileSize
5.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

FileSize
5.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
FileSize
2.71 KB
8.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

FileSize
3.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
8.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
FileSize
3.84 KB
8.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

FileSize
8.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.