Order receipt themed differently for orders entered in admin

I can't reproduce,
every order I have created:
-programmatically
-anonymously
-or via admin/commerce/orders/add

are themed with with my active theme template commerce-order-receipt.html.twig

Forget about my last comment... I must have applied #5 over some leftover...

#5 fixes the receipt theme rendering when order is placed via admin interface.

thanks

+++ b/modules/order/src/EventSubscriber/OrderReceiptSubscriber.php
@@ -120,17 +136,37 @@ class OrderReceiptSubscriber implements EventSubscriberInterface {
+    if (method_exists($this->mailManager, 'getMailTheme')) {
+      $mail_theme = $this->mailManager->getMailTheme();
+      if ($mail_theme && $mail_theme != $current_active_theme->getName()) {
+        $this->themeManager->setActiveTheme($this->themeInitialization->initTheme($mail_theme));
+      }
+    }
...
+      // executed even if an exception is thrown during theming the mail.
+      if ($mail_theme && $mail_theme != $current_active_theme->getName()) {
+        $this->themeManager->setActiveTheme($current_active_theme);
+      }

It's a bummer the mailsystem module doesn't have an interface we can check against.

Can we document that's why we have a method_exists?

I was perplexed why we'd check a method versus an instance. Then I realized it is because of \Drupal\mailsystem\MailsystemManager::getMailTheme.

\Drupal\mailsystem\MailsystemServiceProvider swaps the mail plugin manager.

Feel free to fix and put back to RTBC.

How can we write a test for this?

How can we write a test for this?

We'd need to uhh. Add @requires mailsystem to a test. And. I really don't know, I guess. This requires a theme having an overridden template.

So we would need to:

- Provide a custom theme in the test directory
- Provide an overridden template which has some kind of custom adjustments
- Set the mailsystem to be the custom template, but the current / default template is bartik
- Send email
- Assert content has custom adjustment in template

In this patch, I've added a comment in OrderReceiptSubscriber for the method_exists() usage and written a test, as described in Matt's comment #12.

Interdiff is doing weird things when I try to create the file the correct way, so I'm attaching a "backwards" interdiff for the changes "from" patch #13 "to" patch #5. I've only added to the patch, so just read all the "-" lines as "+" lines. Sorry about that...

+++ b/modules/order/src/EventSubscriber/OrderReceiptSubscriber.php
@@ -117,17 +133,38 @@ class OrderReceiptSubscriber implements EventSubscriberInterface {
+    $mail_theme = FALSE;
...
+      if ($mail_theme && $mail_theme != $current_active_theme->getName()) {
...
+      if ($mail_theme && $mail_theme != $current_active_theme->getName()) {

Instead of false, I feel like we could just do NULL. That way we can remove $mail_theme && checks and it's just "did this equal the returned string".

Yay, we have a test! However, the test needs to be run locally and we need manual testing, anyways from subscribers of this issue.

+  /**
+   * @var \Drupal\Core\Theme\ThemeManagerInterface
+   */
+  protected $themeManager;

Missing one-line description.

-  public function __construct(EntityTypeManagerInterface $entity_type_manager, LanguageManagerInterface $language_manager, MailManagerInterface $mail_manager, OrderTotalSummaryInterface $order_total_summary, Renderer $renderer) {
+  public function __construct(EntityTypeManagerInterface $entity_type_manager, LanguageManagerInterface $language_manager, MailManagerInterface $mail_manager, OrderTotalSummaryInterface $order_total_summary, Renderer $renderer, ThemeManagerInterface $theme_manager, ThemeInitializationInterface $theme_initialization) {

Two new arguments were added to the constructor, but the docblock wasn't expanded with new @param lines.

+    $mail_theme = FALSE;

I agree with Matt, NULL is a more natural default.

+    // Use method_exists() because MailsystemManager doesn't have an interface.

This still didn't make it clear to me. Let's do:

   // The Mail System module swaps out core's MailManager, adding a
    // getMailTheme() method. However, this method is not on any interface.

+        $this->themeManager->setActiveTheme($this->themeInitialization->initTheme($mail_theme));

Nesting two long statements inside each other impacts readability for no gain. Let's do initTheme() on its own line.

+      // Revert the active theme, this is done inside a finally block so it is
+      // executed even if an exception is thrown during theming the mail.

I don't think we should explain PHP syntax here (why/how finally works).

+/**
+ * Tests the sending of order receipt emails.
+ *
+ * @requires module mailsystem
+ * @group commerce
+ */

Needs better description.

+  /**
+   * The Mailsystem settings config object.
+   *
+   * @var \Drupal\Core\Config\Config
+   */
+  protected $mailsystem_config;

Class properties cannot be snake_case. And we aren't even using it in setUp(), so we don't actually need a class property.

+    $this->order->setOrderNumber('2017/01');

This is repeated in both tests, we can just set order_number when constructing the order in setUp().

Thanks Matt and Bojan. Here is the updated patch.

Thanks, looks good now.

+    $this->assertEqual($mailsystem_config->get('theme'), 'current');

I need to fix this pre-commit. assertEqual() is deprecated, we need to use assertEquals() like elsewhere (and the params are reversed).

+ $this->assertEqual($mailsystem_config->get('theme'), 'current');

Oops! Missed that. Thanks for pointing it out. (And I'm always reversing the params...have to remember to double-check that whenever I write tests.)

Thanks, everyone!

Can this one be added in a release? I'm working on the latest dev version and I can't apply this patch with composer. (I guess because of other changes in dev).