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.

CommentFileSizeAuthor
#133 interdiff.txt659 byteskim.pepper
#133 1987848-theme-default-133.patch10.18 KBkim.pepper
#130 interdiff.txt2.25 KBkim.pepper
#130 1987848-theme-default-130.patch10.18 KBkim.pepper
#128 1987848-system-theme-default-128.patch9.82 KBakozma
#126 1987848-system-theme-default-126.patch8.3 KBakozma
#125 1987848-system-theme-default-125.patch8.29 KBakozma
#123 1987848-system-theme-default-123.patch9.11 KBakozma
#115 1987848-system-theme-default-115.patch9.43 KBakozma
#113 1987848-system-theme-default-111.patch13.13 KBakozma
#110 1987848-system-theme-default-110.patch12.95 KBkim.pepper
#107 interdiff.txt12.54 KBkim.pepper
#107 1987848-system-theme-default-107.patch12.97 KBkim.pepper
#97 drupal8.system_theme_default.1987848-97.patch9.35 KBpratik60
#95 drupal8.system_theme_default.1987848-95.patch10 KBpratik60
#92 drupal8.system_theme_default.1987848-92.patch10.08 KBpratik60
#87 drupal8.system_theme_default.1987848-87.patch12.59 KBtlyngej
#83 drupal8.system_theme_default.1987848-83.patch12.42 KBdisasm
#81 drupal8.system_theme_default.1987848-81.patch382.11 KBdisasm
#81 interdiff.txt6.02 KBdisasm
#77 drupal8.system_theme_default.1987848-77.patch9.58 KBdisasm
#77 interdiff.txt2.24 KBdisasm
#75 drupal8.system-module.1987848-75.patch12.05 KBdisasm
#75 interdiff.txt607 bytesdisasm
#71 drupal8.system_theme_default.1987848-71.patch12.1 KBdisasm
#71 interdiff.txt2.36 KBdisasm
#69 drupal8.system_theme_default.1987848-69.patch12.45 KBdisasm
#69 interdiff.txt1.03 KBdisasm
#67 drupal8.system-module.1987848-67.patch12.41 KBdisasm
#67 interdiff.txt6.9 KBdisasm
#65 drupal8.system-module.1987848-65.patch10.52 KBdisasm
#65 interdiff.txt1.21 KBdisasm
#64 1987848-64.patch10.57 KBstar-szr
#63 1987848-63.patch10.53 KBstar-szr
#63 interdiff.txt1.94 KBstar-szr
#62 1987848-62.patch10.52 KBstar-szr
#62 interdiff.txt6.37 KBstar-szr
#58 drupal8.system-module.1987848-58.patch9.39 KBpfrenssen
#58 interdiff.txt671 bytespfrenssen
#55 drupal8.system-module.1987848-55.patch9.44 KBdisasm
#55 interdiff.txt2.56 KBdisasm
#53 drupal8.system_theme.1987848-53.patch9.75 KBdisasm
#51 interdiff-47-49.patch854 bytespfrenssen
#49 system-1987848-49.patch8.72 KBrobeano
#47 interdiff.txt1.03 KBh3rj4n
#47 system-1987848-47.patch8.71 KBh3rj4n
#43 interdiff.txt5.76 KBh3rj4n
#43 system-1987848-43.patch8.6 KBh3rj4n
#37 drupal-convert_system_theme_default-1987848-37.patch9.2 KBParisLiakos
#34 drupal-convert_system_theme_default-1987848-34.patch4.82 KBdisasm
#28 system-route-system-theme-default-1987848-28.patch4.83 KBpwieck
#25 system-route-system-theme-default-1987848-25.patch4.25 KBpwieck
#25 system-route-system-theme-default-1987848-15-25-interdiff.txt459 bytespwieck
#23 1987848-23.patch4.82 KBrgoodine
#15 system-route-system-theme-default-1987848-15.patch4.83 KBpwieck
#11 system-route-system-theme-default-1987848-11.patch7.39 KBpwieck
#8 system-route-system-theme-default-1987848-8.patch7.39 KBInternetDevels
#8 interdiff.txt3.07 KBInternetDevels
#6 system-route-system-theme-default-1987848-6.patch8.9 KBInternetDevels
#1 1987848-route-system-theme-default-1.patch6.66 KBvijaycs85
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vijaycs85’s picture

Status: Active » Needs review
FileSize
6.66 KB

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

Assigned: dtarc » InternetDevels
Issue tags: -WSCCI-conversion +CodeSprintUA

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
FileSize
8.9 KB

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
FileSize
3.07 KB
7.39 KB

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
FileSize
7.39 KB

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
FileSize
4.83 KB

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
FileSize
4.82 KB

Rerolled

Status: Needs review » Needs work

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

pwieck’s picture

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
FileSize
4.83 KB

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

star-szr’s picture

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

star-szr’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:)

star-szr’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
FileSize
4.82 KB

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
FileSize
9.2 KB

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
FileSize
8.6 KB
5.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
FileSize
8.71 KB
1.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
FileSize
8.72 KB

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

Crell’s picture

Interdiff please?

pfrenssen’s picture

FileSize
854 bytes

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
FileSize
9.75 KB

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

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
FileSize
671 bytes
9.39 KB

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.

star-szr’s picture

Assigned: h3rj4n » star-szr

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

star-szr’s picture

Assigned: star-szr » Unassigned
Status: Needs work » Needs review
FileSize
6.37 KB
10.52 KB

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

FileSize
1.94 KB
10.53 KB

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

star-szr’s picture

disasm’s picture

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
FileSize
6.9 KB
12.41 KB

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
FileSize
1.03 KB
12.45 KB

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

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
FileSize
607 bytes
12.05 KB

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

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.

star-szr’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

Proper dependency injection and addresses above comments.

disasm’s picture

Status: Needs work » Needs review
disasm’s picture

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

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

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
FileSize
10 KB

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

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
FileSize
12.97 KB
12.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

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
FileSize
13.13 KB

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

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

akozma’s picture

Status: Needs work » Needs review
akozma’s picture

akozma’s picture

FileSize
8.3 KB

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
FileSize
9.82 KB

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

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
FileSize
10.18 KB
659 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.