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
#133 interdiff.txt659 byteskim.pepper
#133 1987848-theme-default-133.patch10.18 KBkim.pepper
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,774 pass(es).
[ View ]
#130 interdiff.txt2.25 KBkim.pepper
#130 1987848-theme-default-130.patch10.18 KBkim.pepper
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,792 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#128 1987848-system-theme-default-128.patch9.82 KBocsilalala
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,796 pass(es).
[ View ]
#107 1987848-system-theme-default-107.patch12.97 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 59,889 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#97 drupal8.system_theme_default.1987848-97.patch9.35 KBpratik60
PASSED: [[SimpleTest]]: [MySQL] 59,724 pass(es).
[ View ]
#87 drupal8.system_theme_default.1987848-87.patch12.59 KBtlyngej
FAILED: [[SimpleTest]]: [MySQL] 58,285 pass(es), 110 fail(s), and 25 exception(s).
[ View ]
#83 drupal8.system_theme_default.1987848-83.patch12.42 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.system_theme_default.1987848-83.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#81 drupal8.system_theme_default.1987848-81.patch382.11 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.system_theme_default.1987848-81.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#81 interdiff.txt6.02 KBdisasm
#77 drupal8.system_theme_default.1987848-77.patch9.58 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 59,113 pass(es).
[ View ]
#77 interdiff.txt2.24 KBdisasm
#75 drupal8.system-module.1987848-75.patch12.05 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,952 pass(es).
[ View ]
#75 interdiff.txt607 bytesdisasm
#71 drupal8.system_theme_default.1987848-71.patch12.1 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,490 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#71 interdiff.txt2.36 KBdisasm
#69 drupal8.system_theme_default.1987848-69.patch12.45 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,462 pass(es).
[ View ]
#69 interdiff.txt1.03 KBdisasm
#67 drupal8.system-module.1987848-67.patch12.41 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,964 pass(es), 2 fail(s), and 8 exception(s).
[ View ]
#67 interdiff.txt6.9 KBdisasm
#65 drupal8.system-module.1987848-65.patch10.52 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,485 pass(es).
[ View ]
#65 interdiff.txt1.21 KBdisasm
#64 1987848-64.patch10.57 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 58,391 pass(es).
[ View ]
#63 1987848-63.patch10.53 KBCottser
PASSED: [[SimpleTest]]: [MySQL] 58,068 pass(es).
[ View ]
#63 interdiff.txt1.94 KBCottser
#62 1987848-62.patch10.52 KBCottser
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#62 interdiff.txt6.37 KBCottser
#58 drupal8.system-module.1987848-58.patch9.39 KBpfrenssen
FAILED: [[SimpleTest]]: [MySQL] 58,485 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#58 interdiff.txt671 bytespfrenssen
#55 drupal8.system-module.1987848-55.patch9.44 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,415 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#55 interdiff.txt2.56 KBdisasm
#53 drupal8.system_theme.1987848-53.patch9.75 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 58,387 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#51 interdiff-47-49.patch854 bytespfrenssen
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-47-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#49 system-1987848-49.patch8.72 KBrobeano
PASSED: [[SimpleTest]]: [MySQL] 58,066 pass(es).
[ View ]
#47 interdiff.txt1.03 KBh3rj4n
#47 system-1987848-47.patch8.71 KBh3rj4n
PASSED: [[SimpleTest]]: [MySQL] 57,977 pass(es).
[ View ]
#43 interdiff.txt5.76 KBh3rj4n
#43 system-1987848-43.patch8.6 KBh3rj4n
FAILED: [[SimpleTest]]: [MySQL] 57,820 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#37 drupal-convert_system_theme_default-1987848-37.patch9.2 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 56,514 pass(es).
[ View ]
#34 drupal-convert_system_theme_default-1987848-34.patch4.82 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-convert_system_theme_default-1987848-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#28 system-route-system-theme-default-1987848-28.patch4.83 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 58,061 pass(es).
[ View ]
#25 system-route-system-theme-default-1987848-25.patch4.25 KBpwieck
FAILED: [[SimpleTest]]: [MySQL] 58,008 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#25 system-route-system-theme-default-1987848-15-25-interdiff.txt459 bytespwieck
#23 1987848-23.patch4.82 KBrgoodine
PASSED: [[SimpleTest]]: [MySQL] 58,067 pass(es).
[ View ]
#15 system-route-system-theme-default-1987848-15.patch4.83 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 57,980 pass(es).
[ View ]
#11 system-route-system-theme-default-1987848-11.patch7.39 KBpwieck
PASSED: [[SimpleTest]]: [MySQL] 57,767 pass(es).
[ View ]
#8 system-route-system-theme-default-1987848-8.patch7.39 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 56,648 pass(es).
[ View ]
#8 interdiff.txt3.07 KBInternetDevels
#6 system-route-system-theme-default-1987848-6.patch8.9 KBInternetDevels
PASSED: [[SimpleTest]]: [MySQL] 56,050 pass(es).
[ View ]
#1 1987848-route-system-theme-default-1.patch6.66 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 57,017 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new6.66 KB
PASSED: [[SimpleTest]]: [MySQL] 57,017 pass(es).
[ View ]

Initial patch...

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -0,0 +1,68 @@
+ * Definition of Drupal\system\Controller\ThemeController.

Contains ...

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -0,0 +1,68 @@
+ * Controller for Theme handling.

It would be really helpful to describe the controller + method better.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -0,0 +1,68 @@
+   * Menu callback; Set the default theme.

Needs better docs.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -0,0 +1,68 @@
+        // Set the default theme.
+        config('system.theme')
+          ->set('default', $theme)
+          ->save();
...
+        $admin_theme = config('system.theme')->get('admin');

Lets inject the config in order to set it.

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

You could return a RedirectiResponse instead.

+++ b/core/modules/system/system.moduleundefined
@@ -718,10 +718,8 @@ function system_menu() {
   $items['admin/appearance/default'] = array(
     'title' => 'Set default theme',
-    'page callback' => 'system_theme_default',
-    'access arguments' => array('administer themes'),
+    'route_name' => 'system_theme_default',
     'type' => MENU_CALLBACK,
-    'file' => 'system.admin.inc',
   );
   $items['admin/appearance/settings'] = array(

No need for keeping the hook_menu entry.

Status:Needs review» Needs work

.

Assigned:Unassigned» dtarc

I'm going to take a look at this today at drupalcon portland.

As this hasn`t been fixed yet we are going to work on this issue today during Code Sprint UA

Assigned:InternetDevels» Unassigned
Status:Needs work» Needs review
Issue tags:+WSCCI-conversion
StatusFileSize
new8.9 KB
PASSED: [[SimpleTest]]: [MySQL] 56,050 pass(es).
[ View ]

Patch which fixes issues above attached.

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -8,14 +8,20 @@
+ * This implements the Drupal\Core\Controller\ControllerInterface class, adding
+ * special handling for themes.

There seems to be no real reason to document that bit.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -27,13 +33,24 @@ class ThemeController implements ControllerInterface {
+   * The system.theme configFactory object.

I'm not sure whether we want to store the system.theme config object or the full config factory.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -27,13 +33,24 @@ class ThemeController implements ControllerInterface {
+   * @var \Drupal\Core\Config\Config

The config factory is a different class.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -123,4 +142,68 @@ public function enable(Request $request) {
+   * Set the default theme.
+   *
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   A request object containing a theme name and a valid token.
+   *
+   * @return \Symfony\Component\HttpFoundation\RedirectResponse
+   *   Redirects back to the appearance admin page.
+   *
+   * @throws \Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException
+   *   Throws access denied when no theme or token is set in the request or when
+   *   the token is invalid.

Nice!

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -123,4 +142,68 @@ public function enable(Request $request) {
+        $this->configFactory->get('system.theme')
+          ->set('default', $theme)
...
+        $admin_theme = $this->configFactory->get('system.theme')->get('admin');

OH i see now. ... just use $this->config then and get rid of the configFactory object.

Status:Needs work» Needs review
StatusFileSize
new3.07 KB
new7.39 KB
PASSED: [[SimpleTest]]: [MySQL] 56,648 pass(es).
[ View ]

Changes added.

Status:Needs review» Reviewed & tested by the community

Cool stuff!!

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Needs a reroll

curl https://drupal.org/files/system-route-system-theme-default-1987848-8.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7567  100  7567    0     0   7985      0 --:--:-- --:--:-- --:--:--  9814
error: patch failed: core/modules/system/system.routing.yml:122
error: core/modules/system/system.routing.yml: patch does not apply

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

Re-roll. Sorry don't have a handle on making interdiff.txt file yet.

Status:Needs review» Reviewed & tested by the community

The yml file failed, which looks nice now. Back to RTBC

Status:Reviewed & tested by the community» Needs work

Needs a reroll...

curl https://drupal.org/files/system-route-system-theme-default-1987848-11.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  7572  100  7572    0     0   4038      0  0:00:01  0:00:01 --:--:--  5865
error: patch failed: core/modules/system/system.admin.inc:256
error: core/modules/system/system.admin.inc: patch does not apply

Assigned:Unassigned» pwieck

Working on re-roll

Status:Needs work» Needs review
StatusFileSize
new4.83 KB
PASSED: [[SimpleTest]]: [MySQL] 57,980 pass(es).
[ View ]

Re-rolled again.

Status:Needs review» Needs work

The last submitted patch, system-route-system-theme-default-1987848-15.patch, failed testing.

Assigned:pwieck» Unassigned
Issue tags:-Needs reroll

Needs work, removed tag

Status:Needs work» Needs review

#15 passed and ready for review

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Yet another reroll...

curl https://drupal.org/files/system-route-system-theme-default-1987848-15.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  4949  100  4949    0     0   3180      0  0:00:01  0:00:01 --:--:--  6248
error: patch failed: core/modules/system/system.routing.yml:143
error: core/modules/system/system.routing.yml: patch does not apply

Assigned:Unassigned» rgoodine

Working on the reroll

Assigned:rgoodine» Unassigned
Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new4.82 KB
PASSED: [[SimpleTest]]: [MySQL] 58,067 pass(es).
[ View ]

Rerolled

Status:Needs review» Needs work

The last submitted patch, 1987848-23.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new459 bytes
new4.25 KB
FAILED: [[SimpleTest]]: [MySQL] 58,008 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Reroll #23 doesn't look right.

This is being removed in HEAD

-  $items['admin/appearance/default'] = array(
-    'title' => 'Set default theme',
-    'page callback' => 'system_theme_default',
-    'access arguments' => array('administer themes'),
-    'type' => MENU_CALLBACK,
-    'file' => 'system.admin.inc',
-  );
   $items['admin/appearance/settings'] = array(
     'title' => 'Settings',
     'description' => 'Configure default and theme specific settings.',

So this shouldn't be added to patch
diff --git a/core/modules/system/system.routing.yml b/core/modules/system/system.routing.yml
index 45ce1e5..bdfec4f 100644
--- a/core/modules/system/system.routing.yml
+++ b/core/modules/system/system.routing.yml
@@ -157,3 +157,10 @@ system_theme_settings_global:
     _form: '\Drupal\system\Form\ThemeSettingsForm'
   requirements:
     _permission: 'administer themes'
+
+system_theme_default:
+  pattern: '/admin/appearance/default'
+  defaults:
+    _controller: 'Drupal\system\Controller\ThemeController::defaultTheme'
+  requirements:
+    _permission: 'administer themes'

Disregard #25. I don't know what I was even thinking. I just removed what was supposed to be there. :-(

Assigned:Unassigned» pwieck
Status:Needs review» Needs work

As soon as #25 fails I have new file.

Assigned:pwieck» Unassigned
Status:Needs work» Needs review
StatusFileSize
new4.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,061 pass(es).
[ View ]

re-re-re-roll. Minor conflict fixed.

#23: 1987848-23.patch queued for re-testing.

#23 is the patch to review, re-testing now. I worked with @rgoodine yesterday to reroll the patch.

Thanks for working on this @pwieck but I think the patch in #28 is just slightly off - the conflict was a bit tricky on this one.

Snippet from patched system.routing.yml in #28:

system_theme_settings_global:
  pattern: '/admin/appearance/settings/global'
  defaults:
    _form: '\Drupal\system\Form\ThemeSettingsForm'
system_theme_default:
  pattern: '/admin/appearance/default'
  defaults:
    _controller: 'Drupal\system\Controller\ThemeController::defaultTheme'
  requirements:
    _permission: 'administer themes'

system_theme_settings_global is missing 'requirements'. Compare to patched system.routing.yml in #23 where both system_theme_settings_global and system_theme_default specify 'requirements':

system_theme_settings_global:
  pattern: '/admin/appearance/settings/global'
  defaults:
    _form: '\Drupal\system\Form\ThemeSettingsForm'
  requirements:
    _permission: 'administer themes'
system_theme_default:
  pattern: '/admin/appearance/default'
  defaults:
    _controller: 'Drupal\system\Controller\ThemeController::defaultTheme'
  requirements:
    _permission: 'administer themes'

Status:Needs review» Needs work

Issue tags:+Needs reroll

tag:)

Just want to note that normally we reroll the latest patch but in this case we should reroll from #23.

Status:Needs work» Needs review
StatusFileSize
new4.82 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-convert_system_theme_default-1987848-34.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

reroll of #23.

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs reroll

looks good thanks!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal-convert_system_theme_default-1987848-34.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.2 KB
PASSED: [[SimpleTest]]: [MySQL] 56,514 pass(es).
[ View ]

Actually, lets inject url_generator as well

Status:Needs review» Reviewed & tested by the community

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -81,7 +93,7 @@ public function disable(Request $request) {
+      return new RedirectResponse($this->urlGenerator->generateFromPath('admin/appearance', array('absolute' => TRUE)));
@@ -117,10 +129,73 @@ public function enable(Request $request) {
+      return new RedirectResponse($this->urlGenerator->generateFromPath('admin/appearance', array('absolute' => TRUE)));

Just a question for later: do we want to use the route name if existing later or always rely on the path?

i guess using route name would be more solid, cause that would mean someone could change a route url and nothing breaks:)

But you can't replace the route by adding a new one, well these are things which have to be figured out later.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -117,10 +129,73 @@ public function enable(Request $request) {
+          drupal_set_message(t('Please note that the administration theme is still set to the %admin_theme theme; consequently, the theme on this page remains unchanged. All non-administrative sections of the site, however, will show the selected %selected_theme theme by default.', array(
...
+          drupal_set_message(t('%theme is now the default theme.', array('%theme' => $themes[$theme]->info['name'])));
...
+        drupal_set_message(t('The %theme theme was not found.', array('%theme' => $theme)), 'error');

t is injectable... we have the string_translation service on the container..

Assigned:Unassigned» h3rj4n

Issue tags:-CodeSprintUA
StatusFileSize
new8.6 KB
FAILED: [[SimpleTest]]: [MySQL] 57,820 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
new5.76 KB

I removed the 'CodeSprintUA'-tag. I guess after more than a month that sprint is over ;)

I rerolled the patch and changed the translate function to a container. I took \Drupal\system\Form\ModulesListForm as example.

Status:Needs work» Needs review

@@ -27,13 +28,30 @@ class ThemeController implements ControllerInterface {
+  public function __construct(Config $config, PathBasedGeneratorInterface $url_generator, TranslationManager $translation_manager) {
     $this->config = $config;
+    $this->urlGenerator = $url_generator;

Nothing sets the translationManager onto the object.

Status:Needs review» Needs work

The last submitted patch, system-1987848-43.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new8.71 KB
PASSED: [[SimpleTest]]: [MySQL] 57,977 pass(es).
[ View ]
new1.03 KB

This one should work.

Status:Needs review» Needs work
Issue tags:+Novice

@@ -111,16 +133,79 @@ public function enable(Request $request) {
+    $theme = $request->get('theme');
+    $token = $request->get('token');

Don't use $request->get(). Use $request->query->get(), so it's clear where it's coming from.

This is a Novice-able reroll.

Once that's in, though, we should refactor this code entirely. Having the same controller do the page display and the update is very wrong. Having configuration change on a GET request is even more wrong, even if there's a token on it.

At the very least we should split this up into two controllers, one that actually makes the change and then redirects back to the other display-only controller. But I'm OK with that being a follow-up. For now, let's just fix the above and get this in.

Status:Needs work» Needs review
StatusFileSize
new8.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,066 pass(es).
[ View ]

I updated the patch to use $request->query->get() as requested. This could use a review.

Interdiff please?

StatusFileSize
new854 bytes
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-47-49.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's the interdiff between patches from #47 and #49.

Status:Needs review» Needs work

The last submitted patch, interdiff-47-49.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.75 KB
FAILED: [[SimpleTest]]: [MySQL] 58,387 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

replace translationManager with t. Also, cleaned up boilerplate code in controller.

Status:Needs review» Needs work

The last submitted patch, drupal8.system_theme.1987848-53.patch, failed testing.

StatusFileSize
new2.56 KB
new9.44 KB
FAILED: [[SimpleTest]]: [MySQL] 58,415 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, drupal8.system-module.1987848-55.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new671 bytes
new9.39 KB
FAILED: [[SimpleTest]]: [MySQL] 58,485 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I had a look at the test, but it is green for me locally.

I added missing documentation for a parameter.

Status:Needs review» Needs work

The last submitted patch, drupal8.system-module.1987848-58.patch, failed testing.

I can't replicate these errors locally either. If anyone has any suggestions, I'm all ears.

Assigned:h3rj4n» Cottser

The tests are failing locally for me, seeing if I can fix things up.

Assigned:Cottser» Unassigned
Status:Needs work» Needs review
StatusFileSize
new6.37 KB
new10.52 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Well this was fun :)

Main changes:

  • In one of the many rerolls we lost the actual removal of system_theme_default(), fixed.
  • Fixed up use statements and class declaration to the best of my knowledge.
  • ControllerBase doesn't provide $this->request, the request is passed in as an argument (thanks @msonnabaum in IRC!).
  • Wrapped inline comments in defaultTheme() at 80 chars.

StatusFileSize
new1.94 KB
new10.53 KB
PASSED: [[SimpleTest]]: [MySQL] 58,068 pass(es).
[ View ]

Here is the promised comment rewrapping, got left out accidentally.

StatusFileSize
new10.57 KB
PASSED: [[SimpleTest]]: [MySQL] 58,391 pass(es).
[ View ]

StatusFileSize
new1.21 KB
new10.52 KB
PASSED: [[SimpleTest]]: [MySQL] 58,485 pass(es).
[ View ]

Since the text is static, moving drupal_set_title to the routing.yml _title attribute.

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
@@ -111,16 +118,77 @@ public function enable(Request $request) {
+    if (isset($theme) && isset($token) && drupal_valid_token($token, 'system-theme-operation-link')) {

dvt is converted to OO.

Status:Needs work» Needs review
StatusFileSize
new6.9 KB
new12.41 KB
FAILED: [[SimpleTest]]: [MySQL] 58,964 pass(es), 2 fail(s), and 8 exception(s).
[ View ]

Attached patch uses CsrfGenerator service. It also removes ContainerInjection since that comes along with ControllerBase. Finally, using configFactory instead of getting the specific config in the constructor.

Status:Needs review» Needs work

The last submitted patch, drupal8.system-module.1987848-67.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.03 KB
new12.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,462 pass(es).
[ View ]

adding back ContainerInjectionInterface. ControllerBase doesn't implement it.

+++ w/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
@@ -7,33 +7,44 @@
+
...
+   * @param \Drupal\Core\Config\ConfigFactory $config_factory
+   *   Config Factory Service.
...
-    $this->config = $config;
+  public function __construct(ConfigFactory $config_factory, CsrfTokenGenerator $token_generator) {
+    $this->configFactory = $config_factory;
+    $this->tokenGenerator = $token_generator;

You don't have to inject though the config factory as there is a handy config() method on the ControllerBase

StatusFileSize
new2.36 KB
new12.1 KB
FAILED: [[SimpleTest]]: [MySQL] 58,490 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Addressing #70.

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

The last submitted patch, drupal8.system_theme_default.1987848-71.patch, failed testing.

Status:Needs work» Needs review

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

The last submitted patch, drupal8.system_theme_default.1987848-71.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new607 bytes
new12.05 KB
PASSED: [[SimpleTest]]: [MySQL] 58,952 pass(es).
[ View ]

removing config.factory from create().

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.

StatusFileSize
new2.24 KB
new9.58 KB
PASSED: [[SimpleTest]]: [MySQL] 59,113 pass(es).
[ View ]

Fixing redirect() since it's path isn't converted to route yet.

Status:Needs review» Reviewed & tested by the community

Thank you very much!

Status:Reviewed & tested by the community» Needs work

Need to remove function system_theme_default() from system.admin.inc - it's no longer used.

Also… nitpicks:

+++ w/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
@@ -104,23 +80,99 @@ public function enable(Request $request) {
+  /**
+   * Gets Token Generator Service.
+   *
+   * @return \Drupal\Core\Access\CsrfTokenGenerator
+   **/
+  public function getTokenGenerator()
+  {

Extra * at the end of the docblock and the opening brace should not be on its own line. Summary line seems a bit terse and over-capitalized to me, how about "Gets the token generator service." ?

StatusFileSize
new6.02 KB
new382.11 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.system_theme_default.1987848-81.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Proper dependency injection and addresses above comments.

Status:Needs work» Needs review

StatusFileSize
new12.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.system_theme_default.1987848-83.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

forgot to rebase first.

  1. +++ w/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
    @@ -7,45 +7,45 @@
       public static function create(ContainerInterface $container) {
    ...
    +  public function __construct(CsrfTokenGenerator $token_generator) {

    we should swap the order _constructor should be first method.

  2. +++ w/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
    @@ -61,27 +61,29 @@ public static function create(ContainerInterface $container) {
    +      // @todo switch to $this->redirect when admin/appearance converted.
    @@ -104,23 +106,86 @@ public function enable(Request $request) {
    +      // @todo switch to $this->redirect when admin/appearance converted.
    ...
    +      // @todo switch to $this->redirect when admin/appearance converted.

    issue id please.

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

The last submitted patch, drupal8.system_theme_default.1987848-83.patch, failed testing.

StatusFileSize
new12.59 KB
FAILED: [[SimpleTest]]: [MySQL] 58,285 pass(es), 110 fail(s), and 25 exception(s).
[ View ]

Status:Needs work» Needs review

The last submitted patch, drupal8.system_theme_default.1987848-87.patch, failed testing.

The last submitted patch, 81: drupal8.system_theme_default.1987848-81.patch, failed testing.

Issue summary:View changes
StatusFileSize
new10.08 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/system/system.admin.inc.
[ View ]

Rerolling patch

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 92: drupal8.system_theme_default.1987848-92.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10 KB
FAILED: [[SimpleTest]]: [MySQL] 59,281 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Fixing the patch

Status:Needs review» Needs work

The last submitted patch, 95: drupal8.system_theme_default.1987848-95.patch, failed testing.

StatusFileSize
new9.35 KB
PASSED: [[SimpleTest]]: [MySQL] 59,724 pass(es).
[ View ]

Latest patch with all necessary fixes

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 97: drupal8.system_theme_default.1987848-97.patch, failed testing.

The last submitted patch, 97: drupal8.system_theme_default.1987848-97.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Patch (to be ported)

Status:Patch (to be ported)» Needs review

Not sure that "patch to be ported" was the correct status - setting back to needs review.

  1. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
    @@ -60,27 +60,25 @@ public static function create(ContainerInterface $container) {
    +    $token = $request->get('token');
    +    $config = $this->config('system.theme');
    +    if (isset($theme) && isset($token) && $this->tokenGenerator->validate($token, 'system-theme-operation-link')) {

    Controllers should no longer enforce CSRF tokens themselves. Instead, simply add:

    _csrf_token: 'TRUE'

    To the requirements section of the route name, then only use the generator to link to it rather than building the link yourself. The rest is automatic.

  2. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
    @@ -101,24 +99,89 @@ public function disable(Request $request) {
    -      return new RedirectResponse(url('admin/appearance', array('absolute' => TRUE)));
    +      // @todo switch to $this->redirect when admin/appearance converted.
    +      return new RedirectResponse($this->urlGenerator()->generateFromPath('admin/appearance', array('absolute' => TRUE)));

    Is it still not converted?

Status:Needs review» Needs work

I hate the new issue queue workflow...

Status:Needs work» Needs review
StatusFileSize
new12.97 KB
FAILED: [[SimpleTest]]: [MySQL] 59,889 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new12.54 KB

Fixed the csrf tokens, the redirect response, and now using the ThemeHandlerInterface.

The last submitted patch, 107: 1987848-system-theme-default-107.patch, failed testing.

Testing this locally, the links set the default theme, and it displays on the site ok, but the active table in the Block Layout page is still bartik by default.

StatusFileSize
new12.95 KB
FAILED: [[SimpleTest]]: [MySQL] 59,565 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-roll

Status:Needs review» Needs work

The last submitted patch, 110: 1987848-system-theme-default-110.patch, failed testing.

Assigned:Unassigned» ocsilalala

Status:Needs work» Needs review
StatusFileSize
new13.13 KB
FAILED: [[SimpleTest]]: [MySQL] 63,194 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Re-rolling the patch from #110.

I couldn't create an interdiff. After the rebase I added two small changes which were added later to system_theme_default() (this function is removed by the patch).

Adding the line Cache::deleteTags(array('local_task' => TRUE)); after the menu router has rebuild in \Drupal\system\Controller\ThemeController::defaultTheme

Status:Needs review» Needs work

The last submitted patch, 113: 1987848-system-theme-default-111.patch, failed testing.

StatusFileSize
new9.43 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed to enable simpletest module.
[ View ]

Re-roll and update.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 115: 1987848-system-theme-default-115.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 115: 1987848-system-theme-default-115.patch, failed testing.

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 115: 1987848-system-theme-default-115.patch, failed testing.

The last submitted patch, 115: 1987848-system-theme-default-115.patch, failed testing.

StatusFileSize
new9.11 KB
PASSED: [[SimpleTest]]: [MySQL] 64,567 pass(es).
[ View ]

Status:Needs work» Needs review

StatusFileSize
new8.29 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 64,807 pass(es).
[ View ]

Re-roll

StatusFileSize
new8.3 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 65,231 pass(es).
[ View ]

Re-roll

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
@@ -42,18 +42,18 @@ public function disable(Request $request) {
+          $this->get('theme_handler')->disable(array($theme));
@@ -81,17 +81,73 @@ public function enable(Request $request) {
-        theme_enable(array($theme));
...
+          $this->get('theme_handler')->enable(array($theme));

this cant work..there is no get method on $this
theme_handler service should be injected.
which means we have no test coverage there? :(

Status:Needs work» Needs review
StatusFileSize
new9.82 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 66,796 pass(es).
[ View ]

theme_handler injected
$this->get('theme_handler') changed to $this->themeHandler
list_themes() deprecated function changed to $this->themeHandler->listInfo();

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
@@ -77,21 +104,77 @@ public function enable(Request $request) {
+        \Drupal::service('router.builder')->setRebuildNeeded();

This should be injected.

Otherwise this looks good visually.

StatusFileSize
new10.18 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,792 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new2.25 KB

This injects the route.builder service.

its router.builder not route.builder ;)

Status:Needs review» Needs work

The last submitted patch, 130: 1987848-theme-default-130.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new10.18 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,774 pass(es).
[ View ]
new659 bytes

Ahh! We should probably make that more consistent.

Status:Needs review» Reviewed & tested by the community

agreed :)

Checked on simplytest, setting default theme works as should, code looks good => good to go! Thanks!

Status:Reviewed & tested by the community» Fixed

Committed 972b532 and pushed to 8.x. Thanks!

diff --git a/core/modules/system/system.admin.inc b/core/modules/system/system.admin.inc
index 35b10b2..4d6d33a 100644
--- a/core/modules/system/system.admin.inc
+++ b/core/modules/system/system.admin.inc
@@ -10,8 +10,6 @@
use Drupal\Core\Extension\Extension;
use Drupal\Core\Render\Element;
use Drupal\Core\Template\Attribute;
-use Symfony\Component\HttpFoundation\RedirectResponse;
-use Symfony\Component\HttpKernel\Exception\AccessDeniedHttpException;
/**
  * Recursively check compatibility.

Fixed during commit.

  • Commit 972b532 on 8.x by alexpott: Issue #1987848 by disasm, ocsilalala, kim.pepper, pwieck, Cottser,...