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.

Remaining tasks:

  • (novice) create follow-ups (See #90).
Files: 
CommentFileSizeAuthor
#86 1987854-diff-79-86.txt1.57 KBvijaycs85
#86 1987854-system_themes_page-86.patch21.24 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 59,355 pass(es).
[ View ]
#78 1987854-system-theme-page-78.patch21.12 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 59,882 pass(es).
[ View ]
#77 interdiff.txt3.04 KBkim.pepper
#77 1987854-system-theme-page-77.patch20.98 KBkim.pepper
PASSED: [[SimpleTest]]: [MySQL] 59,941 pass(es).
[ View ]
#75 1987854-system-theme-page-75.patch20.99 KBkim.pepper
FAILED: [[SimpleTest]]: [MySQL] 59,872 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#70 1987854-diff-66-70.txt2.44 KBvijaycs85
#70 1987854-system_themes_page-70.patch20.85 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987854-system_themes_page-70.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#68 system-theme-page-1987854-68.patch20.9 KBsplatio
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#68 interdiff.txt2.42 KBsplatio
#66 1987854-diff-64-66.txt1.23 KBvijaycs85
#66 1987854-system_themes_page-66.patch20.58 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 59,056 pass(es).
[ View ]
#64 1987854-diff-61-64.txt1.31 KBvijaycs85
#64 1987854-system_themes_page-64.patch20.56 KBvijaycs85
PASSED: [[SimpleTest]]: [MySQL] 58,995 pass(es).
[ View ]
#61 drupal8.system-module.1987854-61.patch20.96 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,869 pass(es).
[ View ]
#61 interdiff.txt1.23 KBdisasm
#59 system_theme_pages-1987854-59.patch20.78 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 58,626 pass(es).
[ View ]
#59 interdiff.txt1.74 KBsplatio
#57 system_theme_pages-1987854-57.patch20.74 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 58,897 pass(es).
[ View ]
#57 interdiff.txt6.03 KBsplatio
#56 system_theme_pages-1987854-56.patch19.77 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 58,780 pass(es).
[ View ]
#54 system_theme_pages-1987854-54.patch20.05 KBsplatio
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system_theme_pages-1987854-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#54 interdiff.txt1.71 KBsplatio
#52 system_theme_pages-1987854-52.patch18.48 KBsplatio
FAILED: [[SimpleTest]]: [MySQL] 58,894 pass(es), 16 fail(s), and 15 exception(s).
[ View ]
#49 drupal8.system_theme_pages.1987854-49.patch20.24 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,796 pass(es).
[ View ]
#49 interdiff.txt1.05 KBdisasm
#46 drupal8.system_theme_pages.1987854-46.patch20.81 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,676 pass(es).
[ View ]
#46 interdiff.txt1.18 KBdisasm
#44 drupal8.system_theme_pages.1987854-44.patch20.8 KBdisasm
FAILED: [[SimpleTest]]: [MySQL] 59,109 pass(es), 2 fail(s), and 20 exception(s).
[ View ]
#44 interdiff.txt2.29 KBdisasm
#42 drupal8.system-module.1987854-42.patch9.29 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,699 pass(es).
[ View ]
#42 interdiff.txt2.72 KBdisasm
#39 drupal8.system_theme.1987854-39.patch8.49 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,903 pass(es).
[ View ]
#39 interdiff.txt3.51 KBdisasm
#33 drupal8.system_theme_pages.1987854-33.patch20.58 KBdisasm
PASSED: [[SimpleTest]]: [MySQL] 58,031 pass(es).
[ View ]
#33 interdiff.txt8.6 KBdisasm
#32 system_theme_pages-1987854-32.patch21.56 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 58,447 pass(es).
[ View ]
#32 interdiff.txt2.92 KBsplatio
#30 system_theme_pages-1987854-30.patch20.33 KBdawehner
PASSED: [[SimpleTest]]: [MySQL] 58,063 pass(es).
[ View ]
#30 interdiff.txt1.52 KBdawehner
#29 system-theme-page-1987854-29.patch19.87 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 58,061 pass(es).
[ View ]
#29 interdiff.txt1.81 KBsplatio
#27 system-theme-page-1987854-27.patch19.86 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 57,909 pass(es).
[ View ]
#27 interdiff.txt6.63 KBsplatio
#27 interdiff-19-27.txt3.21 KBsplatio
#25 system-theme-page-1987854-25.patch19.88 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 57,898 pass(es).
[ View ]
#25 interdiff.txt8.3 KBsplatio
#19 system-theme-page-1987854-19.patch19.84 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 57,860 pass(es).
[ View ]
#17 system-theme-page-1987854-17.patch19.4 KBsplatio
PASSED: [[SimpleTest]]: [MySQL] 57,519 pass(es).
[ View ]
#17 interdiff.txt6.85 KBsplatio
#15 system-theme-page-1987854-15.patch18.23 KBsplatio
FAILED: [[SimpleTest]]: [MySQL] 57,290 pass(es), 9 fail(s), and 0 exception(s).
[ View ]
#9 system-theme-page-1987854-8.patch17.21 KBsidharthap
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-theme-page-1987854-8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 system-theme-page-1987854-8.patch17.21 KBsidharthap
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-theme-page-1987854-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 system-theme-page-1987854-1.patch11.59 KBsidharthap
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-theme-page-1987854-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new11.59 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-theme-page-1987854-1.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Status:Needs review» Needs work

The last submitted patch, system-theme-page-1987854-1.patch, failed testing.

Assigned:Unassigned» dtarc

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

Assigned:dtarc» E.Kurko
Issue tags:+CodeSprintUA

I'm going to check it today on CodeSprintUA

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

#1: system-theme-page-1987854-1.patch queued for re-testing.

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

The last submitted patch, system-theme-page-1987854-1.patch, failed testing.

StatusFileSize
new17.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-theme-page-1987854-8.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Submitting a fresh patch. But still getting a warning message in local dev.
Invalid argument supplied for foreach() in theme_system_themes_page() (line 1497 of core/modules/system/system.admin.inc).

Status:Needs work» Needs review
StatusFileSize
new17.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system-theme-page-1987854-8_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Changing status.

Status:Needs review» Needs work

The last submitted patch, system-theme-page-1987854-8.patch, failed testing.

Issue tags:+MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

Issue tags:-MENU_LOCAL_ACTION

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

Let's inject the config factory.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.phpundefined
@@ -123,4 +123,139 @@ public function enable(Request $request) {
+      $query['token'] = drupal_get_token('system-theme-operation-link');

Just wondering whether we have a Crypt component in core which has a getToken method and uses everything we need here? (out of scope of this issue for sure).

We have \Drupal\Component\Utility\Crypt, but it only has randomBytes(), hmacBase64(), hashBase64(), and randomStringHashed().

Issue tags:+MENU_LOCAL_ACTION

.

Status:Needs work» Needs review
StatusFileSize
new18.23 KB
FAILED: [[SimpleTest]]: [MySQL] 57,290 pass(es), 9 fail(s), and 0 exception(s).
[ View ]

This is an initial straight conversion that should hopefully get the tests passing. The controller method will need some more cleanup and looking at the tags the local actions converting.

Status:Needs review» Needs work

The last submitted patch, system-theme-page-1987854-15.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new6.85 KB
new19.4 KB
PASSED: [[SimpleTest]]: [MySQL] 57,519 pass(es).
[ View ]

Looks like I spoke too soon! This patch cleans up the documentation, injects the translation service and sets the route name in hook_menu().

  1. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.php
    @@ -0,0 +1,202 @@
    +   * @return string
    +   *  A HTML string of the theme listing page.
    ...
    +    return theme('system_themes_page', array('theme_groups' => $theme_groups, 'theme_group_titles' => $theme_group_titles)) . drupal_render($admin_form);

    Is there any reason to not return a render array instead?

  2. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.php
    @@ -0,0 +1,202 @@
    +      $query['token'] = drupal_get_token('system-theme-operation-link');

    there is now the csrf_token service available to be used.

  3. +++ b/core/modules/system/system.module
    @@ -2911,6 +2909,41 @@ function _system_info_add_path($info, $path) {
    + * Form to select the administration theme.
    + *
    + * @ingroup forms
    + * @see system_themes_admin_form_submit()
    + */
    +function system_themes_admin_form($form, &$form_state, $theme_options) {

    We could think whether this new form should directly be a form controller.

  4. +++ b/core/modules/system/system.module
    @@ -2911,6 +2909,41 @@ function _system_info_add_path($info, $path) {
    +    '#default_value' => config('system.theme')->get('admin'),
    ...
    +  config('system.theme')->set('admin', $form_state['values']['admin_theme'])->save();

    config() should be replaced with Drupal::config()

  5. +++ b/core/modules/system/system.routing.yml
    @@ -130,6 +130,13 @@ system_modules_list_confirm:
    +    _controller: 'Drupal\system\Controller\ThemeListingController::listingPage'

    It should be _content instead of _controller

Assigned:E.Kurko» splatio
StatusFileSize
new19.84 KB
PASSED: [[SimpleTest]]: [MySQL] 57,860 pass(es).
[ View ]

Just gave converting this to a render array a go. For some reason the theme definition in system_theme() is missing an entry for the theme_group_titles variable, meaning it doesn't get passed through to the theme function. Looking at the latest patch from #2009674: Replace theme() with drupal_render() in system module this issue it being fixed there. The other issues in #14 should be addressed.

  1. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.php
    @@ -0,0 +1,214 @@
    +   * @var \Drupal\Core\Extension\ModuleHandler
    ...
    +   * @param \Drupal\Core\Extension\ModuleHandler $module_handler
    ...
    +  public function __construct(Config $config, ModuleHandler $module_handler, TranslationManager $translation, CsrfTokenGenerator $token_generator) {
    +    $this->config = $config;

    It should be ModuleHandlerInterface in these three places so you can swap that theoretically.

  2. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.php
    @@ -0,0 +1,214 @@
    +    return theme('system_themes_page', array('theme_groups' => $theme_groups, 'theme_group_titles' => $theme_group_titles)) . drupal_render($admin_form);

    Let's return a render array instead of render the string.

Here is the parentheses police! These were all present in the original code, but let's clean them up now that we have the chance :)

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.phpundefined
@@ -0,0 +1,214 @@
+        $theme->incompatible_core = !isset($theme->info['core']) || ($theme->info['core'] != DRUPAL_CORE_COMPATIBILITY) || (!isset($theme->info['regions']['content']));

It is not needed to wrap the comparison in the middle in parentheses. Comparison operators such as != have precedence over || and &&.

There's also no need to wrap the final !isset() in parentheses. That is just a single function call.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.phpundefined
@@ -0,0 +1,214 @@
+        $theme->incompatible_base = (isset($theme->info['base theme']) && !isset($themes[$theme->info['base theme']]));

Doesn't need to be wrapped in parentheses. && has precedence over the assignment operator.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.phpundefined
@@ -0,0 +1,214 @@
+        $theme->incompatible_engine = (isset($theme->info['engine']) && !isset($theme->owner));

Same as above, parentheses are not necessary. See PHP operator precedence.

Issue tags:+MENU_LOCAL_ACTION

Did not intend to remove those. I guess we don't need CodeSprintUA any more?

Parentheses also increase readability. It's not just about precedence.

I don't know, for the first one I might agree, but the last two are less readable to me. These look very odd to my eyes, making me pause to inspect them closely to see what kind of trickery is going on in there.

StatusFileSize
new8.3 KB
new19.88 KB
PASSED: [[SimpleTest]]: [MySQL] 57,898 pass(es).
[ View ]

@dawehner on returning a render array @see comment #19 - it feels out of scope to me for this issue to fix system_theme(), but happy to take your lead on that. I'm now using ModuleHandlerInterface but I also along those same lines modified the injected translator to use TranslatorInterface instead of TranslationManager - I'm assuming that was the right move?

@pfrenssen I've removed some of the parentheses but I agree with Tim that in most cases they increase readability.

Actually, it should stay as $this->translation, #2049709: TranslationManager::translate() should be on an interface will be in soon.

StatusFileSize
new3.21 KB
new6.63 KB
new19.86 KB
PASSED: [[SimpleTest]]: [MySQL] 57,909 pass(es).
[ View ]

Argh, just when I thought I was starting to get when to type hint with the interface vs. the class. I'm including an interdiff between #19 and #27 which excludes the cruft from changing the TranslationManager type hint.

Status:Needs work» Needs review
StatusFileSize
new1.81 KB
new19.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,061 pass(es).
[ View ]

Great, this patch replaces the use of TranslationManager with TranslationInterface.

StatusFileSize
new1.52 KB
new20.33 KB
PASSED: [[SimpleTest]]: [MySQL] 58,063 pass(es).
[ View ]

Status:Needs review» Needs work
  1. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.php
    @@ -0,0 +1,223 @@
    +    $admin_form = drupal_get_form('system_themes_admin_form', $admin_theme_options);
    ...
    +    $build[] = $admin_form;

    Could be merged, $admin_form useless

  2. +++ b/core/modules/system/system.admin.inc
    @@ -87,171 +87,6 @@ function system_admin_menu_block_page() {
    -function system_themes_admin_form($form, &$form_state, $theme_options) {
    ...
    -function system_themes_admin_form_submit($form, &$form_state) {
    @@ -330,26 +165,6 @@ function _system_is_incompatible(&$incompatible, $files, $file) {
    -function system_sort_modules_by_info_name($a, $b) {
    ...
    -function system_sort_themes($a, $b) {
    +++ b/core/modules/system/system.module
    @@ -2779,6 +2777,41 @@ function _system_info_add_path($info, $path) {
    +function system_themes_admin_form($form, &$form_state, $theme_options) {
    ...
    +function system_themes_admin_form_submit($form, &$form_state) {
    @@ -2825,6 +2858,26 @@ function system_region_list($theme_key, $show = REGIONS_ALL) {
    +function system_sort_modules_by_info_name($a, $b) {
    ...
    +function system_sort_themes($a, $b) {

    Any reason to move the code around?
    Sort helpers needs more clean-up:
    system_sort_themes() could be a method of the controller

    system_sort_modules_by_info_name() used in controllers that needs to remove module_load_include()

    $ git grep system_sort_modules
    core/modules/system/lib/Drupal/system/Controller/AdminController.php:60:    uasort($module_info, 'system_sort_modules_by_info_name');
    core/modules/system/lib/Drupal/system/Form/ModulesListForm.php:102:    uasort($modules, 'system_sort_modules_by_info_name');
    core/modules/system/lib/Drupal/system/Form/ModulesUninstallForm.php:89:    uasort($disabled, 'system_sort_modules_by_info_name');
    core/modules/system/system.admin.inc:41:  uasort($themes, 'system_sort_modules_by_info_name');
    core/modules/system/system.admin.inc:281:function system_sort_modules_by_info_name($a, $b) {

Status:Needs work» Needs review
StatusFileSize
new2.92 KB
new21.56 KB
PASSED: [[SimpleTest]]: [MySQL] 58,447 pass(es).
[ View ]

Any reason to move the code around?

Initially no (I was just following on from the previous patch), however after looking at system.admin.inc it mainly contains theme functions, controllers and form controllers so will hopefully not be around much longer.

All points should be covered in this patch.

StatusFileSize
new8.6 KB
new20.58 KB
PASSED: [[SimpleTest]]: [MySQL] 58,031 pass(es).
[ View ]

Rerolling for ControllerBase and ContainerInjectionInterface. Getting rid of moduleHandler and translation injections, switching to $this->t() and $this->moduleHandler(). Using configFactory instead of a specific config object because that's what most of core is doing at this point, and not that this class will be extended, but it gets really confusing when you extend a class trying to figure out what $config is the right one.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.php
@@ -0,0 +1,213 @@
+use Drupal\Core\Controller\ControllerBase;
+use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
+use Drupal\Core\Config\ConfigFactory;
+use Drupal\Core\Access\CsrfTokenGenerator;

Sorting order is not correct.

Status:Needs review» Needs work

@jibran - what is the correct sort order for use statements? I had a quick glance at the coding standards but couldn't see anything there.

use Drupal\Core\Access\CsrfTokenGenerator;
use Drupal\Core\Config\ConfigFactory;
use Drupal\Core\Controller\ControllerBase;
use Drupal\Core\DependencyInjection\ContainerInjectionInterface;

We don't have a coding standard for this but alphabetic ascending order with vendor namespaces at the end will work for now.

Status:Needs work» Needs review

I disagree with alphabetical. My preference would be classes the controller extends/implements first, anything injected next, rest of Drupal related stuff, all of vendor stuff for the order. That way it matches the order things appear for the most part in the class itself. Also, when something is added or removed from create, it's easy to find. That being said, since there isn't a standard, lets not hold up the issue because of the sort order. We can always open an issue to add a recommended sort order to the coding standards, just not here.

+++ b/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.php
@@ -0,0 +1,213 @@
+  public function __construct(ConfigFactory $config_factory, CsrfTokenGenerator $token_generator) {
...
+      $container->get('config.factory'),
...
+    $config = $this->configFactory->get('system.theme');

Does the controllerBase provide you $this->config($name) so you don't have to manually inject that?

StatusFileSize
new3.51 KB
new8.49 KB
PASSED: [[SimpleTest]]: [MySQL] 58,903 pass(es).
[ View ]

It does. Also replacing drupal_set_title with _title on route.

  1. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
    @@ -111,16 +84,78 @@ public function enable(Request $request) {
    +   * ¶

    Oh I am so sorry, here is a trailing whitespace.

  2. +++ b/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
    @@ -111,16 +84,78 @@ public function enable(Request $request) {
    +    // Set the page title.

    This comment is invalid/not needed anymore.

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

    Do you also plan to convert drupal_valid_token to the csrf generator service?

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.72 KB
new9.29 KB
PASSED: [[SimpleTest]]: [MySQL] 58,699 pass(es).
[ View ]

Addresses comments in #40.

+++ w/core/modules/system/lib/Drupal/system/Controller/ThemeController.php
@@ -104,23 +78,97 @@ public function enable(Request $request) {
+   **/
+  public function getTokenGenerator()
+  {
+    if (empty($this->tokenGenerator)) {
+      $this->tokenGenerator = $this->container->get('csrf_token');
+    }
+    return $this->tokenGenerator;
+  }

I think we should just use normal injection ... anyway there are multiple codestle issues on here.

StatusFileSize
new2.29 KB
new20.8 KB
FAILED: [[SimpleTest]]: [MySQL] 59,109 pass(es), 2 fail(s), and 20 exception(s).
[ View ]

Goes back to #33. Last few patches were from the wrong issue.

Status:Needs review» Needs work

The last submitted patch, drupal8.system_theme_pages.1987854-44.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.18 KB
new20.81 KB
PASSED: [[SimpleTest]]: [MySQL] 58,676 pass(es).
[ View ]

Forgot a comment... This resolves issue committed here: #2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants

Status:Needs review» Needs work

+++ w/core/modules/system/system.admin.inc
@@ -11,168 +11,25 @@
+    $output = theme('admin_block_content', array('content' => $content));

This is not recommended anymore.

Assigned:splatio» disasm
Status:Needs work» Needs review
StatusFileSize
new1.05 KB
new20.24 KB
PASSED: [[SimpleTest]]: [MySQL] 58,796 pass(es).
[ View ]

That function doesn't exist in core anymore... Removing.

Status:Needs review» Needs work
  1. +++ w/core/modules/system/lib/Drupal/system/Controller/ThemeListingController.php
    @@ -0,0 +1,201 @@
    +    $build[] = drupal_get_form('system_themes_admin_form', $admin_theme_options);

    See drupal_get_form we can convert system_themes_admin_form to OO and pass a class name.

  2. +++ w/core/modules/system/system.module
    @@ -2710,6 +2708,41 @@ function _system_info_add_path($info, $path) {
    + * Form to select the administration theme.
    + *
    + * @ingroup forms
    + * @see system_themes_admin_form_submit()
    + */
    +function system_themes_admin_form($form, &$form_state, $theme_options) {
    +  // Administration theme settings.
    +  $form['admin_theme'] = array(
    +    '#type' => 'details',
    +    '#title' => t('Administration theme'),
    +  );
    +  $form['admin_theme']['admin_theme'] = array(
    +    '#type' => 'select',
    +    '#options' => array(0 => t('Default theme')) + $theme_options,
    +    '#title' => t('Administration theme'),
    +    '#description' => t('Choose "Default theme" to always use the same theme as the rest of the site.'),
    +    '#default_value' => Drupal::config('system.theme')->get('admin'),
    +  );
    +  $form['admin_theme']['actions'] = array('#type' => 'actions');
    +  $form['admin_theme']['actions']['submit'] = array(
    +    '#type' => 'submit',
    +    '#value' => t('Save configuration'),
    +  );
    +  return $form;
    +}
    +
    +/**
    + * Process system_themes_admin_form form submissions.
    + */
    +function system_themes_admin_form_submit($form, &$form_state) {
    +  drupal_set_message(t('The configuration options have been saved.'));
    +  Drupal::config('system.theme')->set('admin', $form_state['values']['admin_theme'])->save();
    +}
    +
    +/**

    Let's convert this form to OO.

You should wait on these conversions until Phase 1 of the new conversions is done, see #1971384-43: [META] Convert page callbacks to controllers

Status:Needs work» Needs review
StatusFileSize
new18.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,894 pass(es), 16 fail(s), and 15 exception(s).
[ View ]

Here is a plain old reroll and doesn't include any items from the reviews above.

Status:Needs review» Needs work

The last submitted patch, system_theme_pages-1987854-52.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new1.71 KB
new20.05 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch system_theme_pages-1987854-54.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Oops - missed injecting the token generator service.

Status:Needs review» Needs work

The last submitted patch, system_theme_pages-1987854-54.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new19.77 KB
PASSED: [[SimpleTest]]: [MySQL] 58,780 pass(es).
[ View ]

Argh ... another reroll!

StatusFileSize
new6.03 KB
new20.74 KB
PASSED: [[SimpleTest]]: [MySQL] 58,897 pass(es).
[ View ]

Converted system_themes_admin_form to FormBase, and replaced the deprecated call to drupal_theme_access with ThemeAccessCheck.

+++ b/core/modules/system/lib/Drupal/system/Form/ThemeAdminForm.php
@@ -0,0 +1,56 @@
+      '#value' => t('Save configuration'),

You can use $this->t()

StatusFileSize
new1.74 KB
new20.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,626 pass(es).
[ View ]

Now using the injected translationManager instead of t().

Status:Needs review» Needs work

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -10,6 +10,9 @@
use Drupal\Core\Controller\ControllerBase;
use Drupal\Core\DependencyInjection\ContainerInjectionInterface;
use Drupal\Core\Entity\Query\QueryFactory;
+use Drupal\Core\Theme\ThemeAccessCheck;
+use Drupal\system\Form\ThemeAdminForm;
+use Drupal\Core\Access\CsrfTokenGenerator;
use Drupal\system\SystemManager;

Keep these in alphabetical order.

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -146,11 +171,144 @@ public function systemAdminMenuBlockPage() {
-   * @todo Remove system_themes_page().
+   * Returns a theme listing.
+   *
+   * @return string
+   *  A HTML string of the theme listing page.
    */

It is "An HTML string" instead of "A HTML string". This line should also be indented with one more space.

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.phpundefined
@@ -161,4 +319,16 @@ public function themeSetDefault() {
+  /**
+   * Array sorting callback; sorts themes by their name.
+   */
+  protected static function systemSortThemes($a, $b) {
+    if ($a->is_default) {
+      return -1;
+    }
+    if ($b->is_default) {
+      return 1;
+    }
+    return strcasecmp($a->info['name'], $b->info['name']);
+  }

Wouldn't it be nicer to use an anonymous function for this?

Status:Needs work» Needs review
StatusFileSize
new1.23 KB
new20.96 KB
PASSED: [[SimpleTest]]: [MySQL] 58,869 pass(es).
[ View ]

Addresses 2 and 3. As for #1, alphabetical is not in the coding standards. If someone wants to open an issue to add it to the coding standards, feel free, but until it's discussed I see no reason to enforce it.

Status:Needs review» Needs work

About the alphabetical ordering, it's not yet in the coding standards, but I saw this mentioned in several issues already. There already is an issue about it as well: #1323082: Establish standards for namespace usage [no patch]. The latest version of the proposed guidelines in that issue mention:

Use statements SHOULD be ordered alphabetically, and Drupal core namespaces should be imported before Drupal extension namespaces, eg:

<?php
use Drupal\Component\Uuid;
use
Drupal\Core\Cache;
use
Drupal\file;
use
Drupal\node;
use
Symfony\Component\HttpFoundation;
?>

So it looks like it will be part of the coding standards in the future, even though it might take a while, that issue has not moved in months. Anyway, you're absolutely right that it is not currently part of the coding standards so it does not need to be addressed for this issue to pass. I just find it a bit sad to mess up a perfectly ordered list.

  1. +++ w/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -146,11 +171,152 @@ public function systemAdminMenuBlockPage() {
    +    uasort($theme_groups['enabled'], function($a, $b) {
    +        if ($a->is_default) {
    +          return -1;
    +        }
    +        if ($b->is_default) {
    +          return 1;
    +        }
    +        return strcasecmp($a->info['name'], $b->info['name']);
    +      });

    The anonymous function is indented 2 spaces too deep.

  2. +++ w/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -161,4 +327,16 @@ public function themeSetDefault() {
    +  /**
    +   * Array sorting callback; sorts themes by their name.
    +   */
    +  protected static function systemSortThemes($a, $b) {
    +    if ($a->is_default) {
    +      return -1;
    +    }
    +    if ($b->is_default) {
    +      return 1;
    +    }
    +    return strcasecmp($a->info['name'], $b->info['name']);
    +  }

    This is replaced by the anonymous function, so it is not needed any more.

  3. +++ w/core/modules/system/system.module
    @@ -2772,6 +2772,13 @@ function system_region_list($theme_key, $show = REGIONS_ALL) {
    /**
    + * Array sorting callback; sorts modules by their name.
    + */
    +function system_sort_modules_by_info_name($a, $b) {
    +  return strcasecmp($a->info['name'], $b->info['name']);
    +}
    +

    This code has moved to another position in the file, but does not seem to have changed in any way, is this intentional?

Issue summary:View changes
Issue tags:+Novice

Status:Needs work» Needs review
StatusFileSize
new20.56 KB
PASSED: [[SimpleTest]]: [MySQL] 58,995 pass(es).
[ View ]
new1.31 KB

Updating changes for review comments in #62

#62.1 - Fixed
#62.2 - Fixed
#62.3 - Not fixed - it is moved from system.admin.inc to system.module to avoid inc include just for this function.

Status:Needs review» Needs work

Just two small points:

Enabling, disabling, and setting an admin theme still works fine.

  1. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -146,11 +171,152 @@ public function systemAdminMenuBlockPage() {
    +      $theme_group_titles['disabled'] = format_plural(count($theme_groups['disabled']), 'Disabled theme', 'Disabled themes');

    Short ads block: #2111349: Move format_plural to the string translation service and format_interval to the date service.

  2. +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -146,11 +171,152 @@ public function systemAdminMenuBlockPage() {
    +    $build[] = drupal_get_form(new ThemeAdminForm(), $admin_theme_options);

    drupal_get_form can now we replaced by the form_builder service.

  3. +++ b/core/modules/system/lib/Drupal/system/Form/ThemeAdminForm.php
    @@ -0,0 +1,56 @@
    +  }
    +}

    New line between them needed.

Status:Needs work» Needs review
StatusFileSize
new20.58 KB
PASSED: [[SimpleTest]]: [MySQL] 59,056 pass(es).
[ View ]
new1.23 KB

Thanks for the review @dawehner. here is the update on them:

#65.1 - NOT FIXED - has to wait until[#2111349] lands
#65.2 - FIXED
#65.3 - FIXED

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
@@ -10,6 +10,9 @@
+use Drupal\system\Form\ThemeAdminForm;
@@ -146,11 +171,152 @@ public function systemAdminMenuBlockPage() {
+    $build[] = \Drupal::formBuilder()->getForm(new ThemeAdminForm(), $admin_theme_options);

You don't need to `use` this or call `new` yourself. Just $this->formBuilder->getForm('Drupal\system\Form\ThemeAdminForm', $admin_theme_options);

And yes, that means you should inject the form builder. Or add it to ControllerBase.

StatusFileSize
new2.42 KB
new20.9 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

This should fix the issues raised in #67.

Status:Needs review» Needs work

The last submitted patch, 68: system-theme-page-1987854-68.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.85 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987854-system_themes_page-70.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new2.44 KB

updating changes for #67.

The last submitted patch, 70: 1987854-system_themes_page-70.patch, failed testing.

Issue tags:+Needs reroll

Issue tags:-Needs reroll
StatusFileSize
new20.99 KB
FAILED: [[SimpleTest]]: [MySQL] 59,872 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Re-roll.

Status:Needs review» Needs work

The last submitted patch, 75: 1987854-system-theme-page-75.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new20.98 KB
PASSED: [[SimpleTest]]: [MySQL] 59,941 pass(es).
[ View ]
new3.04 KB

Fixes issue with CSRF token by using route_names instead of urls. Two birds with one stone?!

StatusFileSize
new21.12 KB
PASSED: [[SimpleTest]]: [MySQL] 59,882 pass(es).
[ View ]
new4.47 KB

This cleans up the themesPage() a little bit more:

  • Removed the dependecy on CsrfTokenGenerator as we aren't using it any more
  • Added a dependency on ThemeHandler so we can use it for getting the theme list (replaces procedural call)
  • Depend on FormBuilderInterface instead of FormBuilder
  • Replace calls to deprecated format_plural with use of translationManager() in ControllerBase

StatusFileSize
new21.16 KB
PASSED: [[SimpleTest]]: [MySQL] 60,126 pass(es).
[ View ]
new602 bytes

Added missing comment on $themeHandler property.

Status:Needs review» Reviewed & tested by the community
  • +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -146,11 +179,152 @@ public function systemAdminMenuBlockPage() {
    +    $themes = $this->themeHandler->listInfo();

    It is great to see it in use!

  • "Re-test request ignored because test is already queued."

    Status:Reviewed & tested by the community» Needs work

    +++ b/core/modules/system/system.admin.inc
    @@ -264,26 +88,6 @@ function _system_is_incompatible(&$incompatible, $files, $file) {
    -}
    ...
    -/**
    - * Array sorting callback; sorts modules or themes by their name.
    - */
    -function system_sort_themes($a, $b) {
    -  if ($a->is_default) {
    -    return -1;
    -  }
    -  if ($b->is_default) {
    -    return 1;
    -  }
    -  return strcasecmp($a->info['name'], $b->info['name']);
    +++ b/core/modules/system/lib/Drupal/system/Controller/SystemController.php
    @@ -146,11 +181,152 @@ public function systemAdminMenuBlockPage() {
    +    uasort($theme_groups['enabled'], function($a, $b) {
    +      if ($a->is_default) {
    +        return -1;
    +      }
    +      if ($b->is_default) {
    +        return 1;
    +      }
    +      return strcasecmp($a->info['name'], $b->info['name']);
    +    });
    +    $this->moduleHandler()->alter('system_themes_page', $theme_groups);

    This all looks good, except this part...

    I don't understand why this was done. We introduced an API change and also removed the documentation (such that it was) that explains what this convoluted code is doing.

    I think we should do what we did to system_sort_modules_by_info_name() here and simply move the function to system.module for now. Then probably a follow-up to discuss the fate of these functions. (For example, they probably make more sense off the ModuleHandler class or whatever the equivalent of that is for themes.) It's out of scope for this issue though, IMO.

    We could just document the anonymous function better and actually explain what its doing. That's some weird logic and it should have that anyways instead of just lying about sorting by name.

    We could, but it's not part of converting this from a page controller to a controller controller, so let's discuss it in a separate issue.

    Status:Needs work» Needs review
    StatusFileSize
    new21.24 KB
    PASSED: [[SimpleTest]]: [MySQL] 59,355 pass(es).
    [ View ]
    new1.57 KB

    Fixing #83....

    Status:Needs review» Reviewed & tested by the community

    This resolves the issues in #83 so back to RTBC

    Status:Reviewed & tested by the community» Fixed

    Great, thanks!

    Committed and pushed to 8.x.

    Status:Fixed» Closed (fixed)

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

    Assigned:disasm» Unassigned
    Issue summary:View changes
    Issue tags:+Needs followup

    I didn't read all the comments on this issue, but I did see at least in #83 and #84 one or two (I'm not sure) follow-ups are asked for.
    tagging so we dont forget.

    could be a novice issue to read through the comments and identify other issues that need to be created, and link them back to this issue (using the related meta data field on issues, and also just making a comment here that they were created. that will help the people who are familiar with this issue look at the new issues to make sure the summaries are accurate.)

    when someone reads all the comments and verifies all follow-ups are created, the needs follow-up tag should be removed.

    the dreditor clone button can be helpful for autofilling similar components and such. note that new follow-ups in this case should probably be status active.

    Title:Convert system_themes_page() to a new style controller[fixed needs follow-ups] Convert system_themes_page() to a new style controller
    Status:Closed (fixed)» Active

    I'm not sure what "status" to use for this.
    trying this with a note in the title that the issue is fixed.