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).
CommentFileSizeAuthor
#86 1987854-diff-79-86.txt1.57 KBvijaycs85
#86 1987854-system_themes_page-86.patch21.24 KBvijaycs85
#79 interdiff.txt602 byteskim.pepper
#79 1987854-system-theme-page-79.patch21.16 KBkim.pepper
#78 interdiff.txt4.47 KBkim.pepper
#78 1987854-system-theme-page-78.patch21.12 KBkim.pepper
#77 interdiff.txt3.04 KBkim.pepper
#77 1987854-system-theme-page-77.patch20.98 KBkim.pepper
#75 1987854-system-theme-page-75.patch20.99 KBkim.pepper
#70 1987854-diff-66-70.txt2.44 KBvijaycs85
#70 1987854-system_themes_page-70.patch20.85 KBvijaycs85
#68 system-theme-page-1987854-68.patch20.9 KBmsmithcti
#68 interdiff.txt2.42 KBmsmithcti
#66 1987854-diff-64-66.txt1.23 KBvijaycs85
#66 1987854-system_themes_page-66.patch20.58 KBvijaycs85
#64 1987854-diff-61-64.txt1.31 KBvijaycs85
#64 1987854-system_themes_page-64.patch20.56 KBvijaycs85
#61 drupal8.system-module.1987854-61.patch20.96 KBdisasm
#61 interdiff.txt1.23 KBdisasm
#59 system_theme_pages-1987854-59.patch20.78 KBmsmithcti
#59 interdiff.txt1.74 KBmsmithcti
#57 system_theme_pages-1987854-57.patch20.74 KBmsmithcti
#57 interdiff.txt6.03 KBmsmithcti
#56 system_theme_pages-1987854-56.patch19.77 KBmsmithcti
#54 system_theme_pages-1987854-54.patch20.05 KBmsmithcti
#54 interdiff.txt1.71 KBmsmithcti
#52 system_theme_pages-1987854-52.patch18.48 KBmsmithcti
#49 drupal8.system_theme_pages.1987854-49.patch20.24 KBdisasm
#49 interdiff.txt1.05 KBdisasm
#46 drupal8.system_theme_pages.1987854-46.patch20.81 KBdisasm
#46 interdiff.txt1.18 KBdisasm
#44 drupal8.system_theme_pages.1987854-44.patch20.8 KBdisasm
#44 interdiff.txt2.29 KBdisasm
#42 drupal8.system-module.1987854-42.patch9.29 KBdisasm
#42 interdiff.txt2.72 KBdisasm
#39 drupal8.system_theme.1987854-39.patch8.49 KBdisasm
#39 interdiff.txt3.51 KBdisasm
#33 drupal8.system_theme_pages.1987854-33.patch20.58 KBdisasm
#33 interdiff.txt8.6 KBdisasm
#32 system_theme_pages-1987854-32.patch21.56 KBmsmithcti
#32 interdiff.txt2.92 KBmsmithcti
#30 system_theme_pages-1987854-30.patch20.33 KBdawehner
#30 interdiff.txt1.52 KBdawehner
#29 system-theme-page-1987854-29.patch19.87 KBmsmithcti
#29 interdiff.txt1.81 KBmsmithcti
#27 system-theme-page-1987854-27.patch19.86 KBmsmithcti
#27 interdiff.txt6.63 KBmsmithcti
#27 interdiff-19-27.txt3.21 KBmsmithcti
#25 system-theme-page-1987854-25.patch19.88 KBmsmithcti
#25 interdiff.txt8.3 KBmsmithcti
#19 system-theme-page-1987854-19.patch19.84 KBmsmithcti
#17 system-theme-page-1987854-17.patch19.4 KBmsmithcti
#17 interdiff.txt6.85 KBmsmithcti
#15 system-theme-page-1987854-15.patch18.23 KBmsmithcti
#9 system-theme-page-1987854-8.patch17.21 KBsidharthap
#8 system-theme-page-1987854-8.patch17.21 KBsidharthap
#1 system-theme-page-1987854-1.patch11.59 KBsidharthap
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sidharthap’s picture

Status: Active » Needs review
FileSize
11.59 KB

Status: Needs review » Needs work

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

dtarc’s picture

Assigned: Unassigned » dtarc

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

andypost’s picture

E.Kurko’s picture

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

I'm going to check it today on CodeSprintUA

E.Kurko’s picture

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.

sidharthap’s picture

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

sidharthap’s picture

Status: Needs work » Needs review
FileSize
17.21 KB

Changing status.

Status: Needs review » Needs work

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

tim.plunkett’s picture

Issue tags: +MENU_LOCAL_ACTION

This blocks removal of MENU_LOCAL_ACTION.

dawehner’s picture

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

tim.plunkett’s picture

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

tim.plunkett’s picture

Issue tags: +MENU_LOCAL_ACTION

.

msmithcti’s picture

Status: Needs work » Needs review
FileSize
18.23 KB

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.

msmithcti’s picture

Status: Needs work » Needs review
FileSize
6.85 KB
19.4 KB

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

dawehner’s picture

  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

msmithcti’s picture

Assigned: E.Kurko » msmithcti
FileSize
19.84 KB

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.

dawehner’s picture

  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.

pfrenssen’s picture

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.

pfrenssen’s picture

Issue tags: +MENU_LOCAL_ACTION

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

tim.plunkett’s picture

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

pfrenssen’s picture

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.

msmithcti’s picture

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

tim.plunkett’s picture

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

msmithcti’s picture

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.

tim.plunkett’s picture

Status: Needs review » Needs work
msmithcti’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
19.87 KB

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

dawehner’s picture

Second point of #1987854-20: Convert system_themes_page() to a new style controller is still true, here is a simple fix.

andypost’s picture

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) {
    
msmithcti’s picture

Status: Needs work » Needs review
FileSize
2.92 KB
21.56 KB

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.

disasm’s picture

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.

jibran’s picture

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

msmithcti’s picture

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.

jibran’s picture

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.

disasm’s picture

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.

dawehner’s picture

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

disasm’s picture

It does. Also replacing drupal_set_title with _title on route.

dawehner’s picture

  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?

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

Addresses comments in #40.

dawehner’s picture

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

disasm’s picture

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.

disasm’s picture

Status: Needs work » Needs review
FileSize
1.18 KB
20.81 KB
disasm’s picture

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

jibran’s picture

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.

disasm’s picture

Assigned: msmithcti » disasm
Status: Needs work » Needs review
FileSize
1.05 KB
20.24 KB

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

jibran’s picture

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.

tim.plunkett’s picture

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

msmithcti’s picture

Status: Needs work » Needs review
FileSize
18.48 KB

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.

msmithcti’s picture

Status: Needs work » Needs review
FileSize
1.71 KB
20.05 KB

Oops - missed injecting the token generator service.

Status: Needs review » Needs work

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

msmithcti’s picture

Status: Needs work » Needs review
FileSize
19.77 KB

Argh ... another reroll!

msmithcti’s picture

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

tim.plunkett’s picture

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

You can use $this->t()

msmithcti’s picture

Now using the injected translationManager instead of t().

pfrenssen’s picture

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?

disasm’s picture

Status: Needs work » Needs review
FileSize
1.23 KB
20.96 KB

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.

pfrenssen’s picture

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:

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?

heddn’s picture

Issue summary: View changes
Issue tags: +Novice
vijaycs85’s picture

Status: Needs work » Needs review
FileSize
20.56 KB
1.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.

dawehner’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
20.58 KB
1.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

tim.plunkett’s picture

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

msmithcti’s picture

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.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
20.85 KB
2.44 KB

updating changes for #67.

tim.plunkett’s picture

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

kim.pepper’s picture

Issue tags: +Needs reroll
kim.pepper’s picture

kim.pepper’s picture

Issue tags: -Needs reroll
FileSize
20.99 KB

Re-roll.

Status: Needs review » Needs work

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

kim.pepper’s picture

Status: Needs work » Needs review
FileSize
20.98 KB
3.04 KB

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

kim.pepper’s picture

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

Added missing comment on $themeHandler property.

dawehner’s picture

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!

  • xjm’s picture

    kim.pepper’s picture

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

    webchick’s picture

    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.

    neclimdul’s picture

    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.

    webchick’s picture

    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.

    vijaycs85’s picture

    Status: Needs work » Needs review
    FileSize
    21.24 KB
    1.57 KB

    Fixing #83....

    kim.pepper’s picture

    Status: Needs review » Reviewed & tested by the community

    This resolves the issues in #83 so back to RTBC

    webchick’s picture

    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.

    YesCT’s picture

    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.

    YesCT’s picture

    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.

    andypost’s picture

    #83 is addressed in commited patch. So it's not clear what's left here

    alexpott’s picture

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

    Yep #86 reverted the contentious changes.

    Status: Fixed » Closed (fixed)

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