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 KBakozma
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

vijaycs85’s picture

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

Initial patch...

dawehner’s picture

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

dawehner’s picture

Status:Needs review» Needs work

.

dtarc’s picture

Assigned:Unassigned» dtarc

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

InternetDevels’s picture

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

InternetDevels’s picture

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.

dawehner’s picture

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.

InternetDevels’s picture

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

Changes added.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Cool stuff!!

alexpott’s picture

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
pwieck’s picture

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.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

alexpott’s picture

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
pwieck’s picture

Assigned:Unassigned» pwieck

Working on re-roll

pwieck’s picture

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.

pwieck’s picture

Assigned:pwieck» Unassigned
Issue tags:-Needs reroll

Needs work, removed tag

pwieck’s picture

Status:Needs work» Needs review
pwieck’s picture

#15 passed and ready for review

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Back to RTBC.

alexpott’s picture

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
rgoodine’s picture

Assigned:Unassigned» rgoodine

Working on the reroll

rgoodine’s picture

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.

pwieck’s picture

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'
pwieck’s picture

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

pwieck’s picture

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

As soon as #25 fails I have new file.

pwieck’s picture

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.

Cottser’s picture

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

Cottser’s picture

#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'

ParisLiakos’s picture

Status:Needs review» Needs work
ParisLiakos’s picture

Issue tags:+Needs reroll

tag:)

Cottser’s picture

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

disasm’s picture

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.

ParisLiakos’s picture

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.

ParisLiakos’s picture

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

Actually, lets inject url_generator as well

dawehner’s picture

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?

ParisLiakos’s picture

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

dawehner’s picture

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

alexpott’s picture

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

h3rj4n’s picture

Assigned:Unassigned» h3rj4n
h3rj4n’s picture

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.

h3rj4n’s picture

Status:Needs work» Needs review
dawehner’s picture

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

h3rj4n’s picture

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

This one should work.

Crell’s picture

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.

robeano’s picture

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.

Crell’s picture

Interdiff please?

pfrenssen’s picture

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.

disasm’s picture

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.

disasm’s picture

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

Status:Needs work» Needs review

Status:Needs review» Needs work

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

pfrenssen’s picture

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.

disasm’s picture

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

Cottser’s picture

Assigned:h3rj4n» Cottser

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

Cottser’s picture

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.
Cottser’s picture

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.

Cottser’s picture

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

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.

jibran’s picture

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.

disasm’s picture

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.

disasm’s picture

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.

dawehner’s picture

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

disasm’s picture

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.

dawehner’s picture

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.

disasm’s picture

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

removing config.factory from create().

xjm’s picture

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.

disasm’s picture

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.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

Thank you very much!

alexpott’s picture

Status:Reviewed & tested by the community» Needs work

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

Cottser’s picture

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." ?

disasm’s picture

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.

disasm’s picture

Status:Needs work» Needs review
disasm’s picture

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.

jibran’s picture

  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.

googletorp’s picture

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

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

tlyngej’s picture

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

Status:Needs work» Needs review

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

pratik60’s picture

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

pratik60’s picture

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

Rerolling patch

pratik60’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

pratik60’s picture

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.

pratik60’s picture

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

Latest patch with all necessary fixes

pratik60’s picture

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.

pratik60’s picture

Status:Needs work» Needs review
pratik60’s picture

Status:Needs review» Patch (to be ported)
alexpott’s picture

Status:Patch (to be ported)» Needs review

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

dawehner’s picture

Crell’s picture

  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?

Crell’s picture

Status:Needs review» Needs work

I hate the new issue queue workflow...

kim.pepper’s picture

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.

kim.pepper’s picture

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.

kim.pepper’s picture

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.

akozma’s picture

Assigned:Unassigned» akozma
akozma’s picture

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.

akozma’s picture

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

Re-roll and update.

akozma’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

akozma’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

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

akozma’s picture

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.

akozma’s picture

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

Status:Needs work» Needs review
akozma’s picture

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

Re-roll

akozma’s picture

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

Re-roll

ParisLiakos’s picture

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? :(

akozma’s picture

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();

Crell’s picture

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

kim.pepper’s picture

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.

ParisLiakos’s picture

its router.builder not route.builder ;)

Status:Needs review» Needs work

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

kim.pepper’s picture

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.

ParisLiakos’s picture

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!

alexpott’s picture

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

Status:Fixed» Closed (fixed)

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