Support from Acquia helps fund testing for Drupal Acquia logo

Comments

shkiper’s picture

Issue summary: View changes
FileSize
27.11 KB

Hi, I made a version of this module which works with current Drupal 8 release
Here is my patch

mgifford’s picture

Title: D8 port. » Port to Drupal 8 Please
calebtr’s picture

Issue summary: View changes
Status: Needs review » Needs work
FileSize
27.33 KB

I got Switchtheme working for Drupal 8, kinda sorta, see issues I noticed below.

This patch still applies to 7.x-1.x-dev - thanks @andrii zahura@. I updated the issue summary.

I added a couple of functions for classes that needed getEditableConfigNames().

The form now checks that a theme is not labeled "hidden" - this keeps Classy and Stable out of the form choices. I'm not sure I did this correctly or if we should be using a getter:

src/Switchtheme.php:

      if (\Drupal::service('access_check.theme')->checkAccess($theme_name) &! $theme->info['hidden']) {

Issues I noticed:

  1. The patch has about 300 of whitespace errors - mostly ^M/pagebreak instead of newline
  2. Theme switching only works for anonymous users (once given permission) - for authenticated users, the setting isn't saved
  3. Link to config page doesn't show up at admin/config
  4. Config form needs better instructions & should allow admin to choose which themes are available to switch to
  5. Switchtheme block has two submit buttons

I didn't test the random theme block at all.

I am attaching an updated patch. It would be great to get this committed as a 8.x-1.x-dev branch so we could track it better and file some more specific issues.

shkiper’s picture

Status: Needs work » Needs review
FileSize
27.47 KB

Hi @calebtr, thank you for your review. I fixed issues from your comment

TimeBandit’s picture

Thanks for the port! Here are some comments:

  1. Link to config page doesn't show up at admin/config. Config is here: /admin/config/user-interface/switchtheme
  2. needs switchtheme.install file or it can’t be uninstalled, even if hook is empty:
    https://www.drupal.org/node/2225029#comment-10108294

    function switchtheme_uninstall() {
    }
  3. on admin/config/user-interface/switchtheme i get an error for each theme, I believe because no labels are set initially. Example:
    “Notice: Undefined index: switchtheme_bartik in Drupal\switchtheme\Form\SwitchthemeAdminSettings->buildForm() (line 46 of modules/contrib/switchtheme/src/Form/SwitchthemeAdminSettings.php”
  4. after enabling Switchtheme block I see the dropdown and “switch” button but no labels on select options unless I go back to Switchtheme Admin and enter labels. Should module force labels? Perhaps the default for labels should be the name each is already given, though ability to override as provided is nice.
  5. Admin should be able to limit theme choices. Dreaming big would be different sets; a block for each grouping/set of themes, meaning you could have one Switchtheme block that shows 2 out of 7 Admin-selected themes, another block that shows all 7, etc., in the event different roles need different role choice groups.
  6. permissions check seems wrong. Themes that I don’t have enabled under Appearance show up in Switchtheme dropdown options. This touches on the feature of being able to limit themes, but it should be a given that user cannot choose themes disabled under Appearance in any event because these themes cannot even assign the Switchtheme block.
shkiper’s picture

Hi @TimeBandit. Thank you for your review.
I have fixed issues 1,3,4,6.
2- I can uninstall module without switchtheme.install file
5 - it is great idea, I'll try to implement this feature in future, but it needs some time

mgifford’s picture

Ok.. So can we put out a rough D8 dev release with the code in #6?

Just like to start nudging this closer to a release. Or should we just start recommending folks move to something like https://www.drupal.org/project/styleswitcher for D8?

OFF’s picture

Any news?