In Panopoly 1.x, we had panopoly_theme.profile.inc that we could include a distro's installer and call a couple functions to add a theme selector step to the installation process. This issue is to port that to Drupal 8!

CommentFileSizeAuthor
#5 interdiff_1_5.txt1.62 KBshadcn
#5 panopoly-theme-selector-2728715-5.patch10.63 KBshadcn
#2 panopoly-theme-selection.gif186.36 KBshadcn
#2 panopoly-theme-selector-2728715-1.patch11.06 KBshadcn
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dsnopek created an issue. See original summary.

shadcn’s picture

shadcn CreditAttribution: shadcn at Chapter Three commented
Status: Active » Needs review
FileSize
panopoly-theme-selector-2728715-1.patch11.06 KB
panopoly-theme-selection.gif186.36 KB

Here's a first patch that adds the theme selection install tasks.

Theme Selection

Adding this to a custom distro's installer is same as before:

// Add the Panopoly theme selection to the installation process
module_load_include('inc', 'panopoly_theme', 'panopoly_theme.profile');
$tasks += panopoly_theme_profile_theme_selection_install_task($install_state);
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented

Awesome! Works perfect in my testing. :-)

However, I have one question/concern from looking at the code: This patch moves the panopoly_install_tasks(() from panopoly.install to panopoly.profile. That's where we had it in Panopoly 1.x, but my impression was that it was better to put in the .install file because that's only loaded during installation, whereas the .profile file is loaded always (like a .module file) under the normal operation of the site. That said, I don't know if that's actually accurate. :-)

Do you know if I'm right or wrong about that? Is there a reason you had to move it from panopoly.install to panopoly.profile? If that's necessary to make this work, that will decide this for us.

Thanks!

shadcn’s picture

shadcn CreditAttribution: shadcn at Chapter Three commented

I looked into this a bit more.

I remember back in Drupal 7 we had some issues with hook_install_tasks in .install and Batch API. The fix was to put it in .profile. I believe this has been fixed now.

Looking at the code of some major distributions it seems they all implement this hook in .profile. I've been doing the same but I don't see any documentation backing this up.

Moreover, I ran some tests to check if .profile is still loaded on every request and it still is.

But here's an interesting piece of code I found in install.core.inc:


function install_tasks($install_state) {
  ...
  // Now add any tasks defined by the installation profile.
  if (!empty($install_state['parameters']['profile'])) {
    // Load the profile install file, because it is not always loaded when
    // hook_install_tasks() is invoked (e.g. batch processing).
    $profile = $install_state['parameters']['profile'];
    $profile_install_file = $install_state['profiles'][$profile]->getPath() . '/' . $profile . '.install';
    if (file_exists($profile_install_file)) {
      include_once \Drupal::root() . '/' . $profile_install_file;
    }
    $function = $install_state['parameters']['profile'] . '_install_tasks';
    if (function_exists($function)) {
      $result = $function($install_state);
      if (is_array($result)) {
        $tasks += $result;
      }
    }
  }
  ...

It looks like Drupal assumes hook_install_tasks is in .install 🤔

shadcn’s picture

shadcn CreditAttribution: shadcn at Chapter Three commented
FileSize
panopoly-theme-selector-2728715-5.patch10.63 KB
interdiff_1_5.txt1.62 KB

Here's an updated patch.

  • dsnopek committed 8f82d18 on 8.x-2.x authored by arshadcn 
    Issue #2728715 by arshadcn, dsnopek: Implement reusable theme selector...
dsnopek’s picture

dsnopek
he/him
Primary language English
Location USA
CreditAttribution: dsnopek commented
Status: Needs review » Fixed

Thanks! Committed :-)

Status: Fixed » Closed (fixed)

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