Support for Externalized Template Files

The default templates are in subscriptions_mail.templates.inc. How about allowing a subscriptions_mail.custom_templates.inc file to override the former, if it's found?

I'm not sure about putting it into the themes directory. The file contains executable code that runs in the module context. Running code from the themes directory in the module context seems to be a violation of Drupal's execution model. It might be a security breach.

Have you reported the problems with Mail Editor? Maybe I've forgotten, but I'd like to look into those now in #651304: subscriptions_mail_digest template??. I'd appreciate any insight you can offer there.

How about allowing a subscriptions_mail.custom_templates.inc file to override the former, if it's found?

That would be ok for me, if it would still freeze the mail template compatibility for the future (even if there were changes?).

That is difficult. We could implement a version number for the template file, so that you'd at least get an alert when the default template file has changed and you need to update your customized version.

Your attached file is a theming file. It runs in the theming context: it has access to nothing but the documented "pre-cooked" variables, and it runs only after all of the processing is done. It has much less potential to mess up something and thus can be entrusted to themers. Module files should not be entrusted to themers. (This separation may change though.)

Thanks for the info about Mail Editor.

+++ subscriptions.module	2012-03-11 12:58:36.000000000 -0700
@@ -649,3 +649,16 @@
+ * Substitute a possible external template file instead of the default.

This is YOUR intention with the "then" branch (= your change). We want to describe the entire function, i.e. write something like "Load the default templates." (default as in not-customized-in-MailEdit)

Add something like "The Subscriptions default templates can be overridden with site-specific default templates by placing a file named '...' into the '...' directory."

+++ subscriptions.module	2012-03-11 12:58:36.000000000 -0700
@@ -649,3 +649,16 @@
+function subscriptions_template_chooser() {

Move this to the subscriptions_mail.module and call it for what it does, e.g. subscriptions_mail_load_default_templates(). Maybe you can even move it into a subscriptions_mail*.inc file — that would be even better...

+++ subscriptions.module	2012-03-11 12:58:36.000000000 -0700
@@ -649,3 +649,16 @@
+   global $theme_path;

Drupal convention is to indent two spaces per level, not three.

+++ subscriptions.module	2012-03-11 12:58:36.000000000 -0700
@@ -649,3 +649,16 @@
+   if (is_file($theme_path .'/subscriptions_mail.templates.inc')) {
+      include_once $theme_path .'/subscriptions_mail.templates.inc';
+   }
+   else {
+      include_once drupal_get_path('module', 'subscriptions_mail') .'/subscriptions_mail.templates.inc';
+   }

Try to avoid duplication. Something like

+  $file = 'subscriptions_mail.templates.inc';
+  if (is_file($path = $theme_path .'/'. $file)) {
+    include_once $path;
+  }
+  else {
+    include_once drupal_get_path('module', 'subscriptions_mail') .'/'. $file;
+  }

+++ subscriptions_content.module	2012-03-11 13:01:18.000000000 -0700
@@ -606,7 +606,7 @@
 function _subscriptions_content_node_mailvars(&$mailvars, $node, $field, $s) {
   global $language;
-  include_once drupal_get_path('module', 'subscriptions_mail') .'/subscriptions_mail.templates.inc';
+  subscriptions_template_chooser();

Is it really necessary to load the templates here in subscriptions_content.module? Could we avoid it? Maybe by pre-loading them where we call this function from.

D7 needs to go first here...

Yes, that's how it goes all over d.o. We cannot have fixes/additions in old versions (such as D6) that disappear when you upgrade (to D7 in this case). Upgrading is hard enough — we must not put ourselves in a position where we willingly cause old issues to come back to life for upgraders. I'm sure you'd hate that if you were in that position.

Ripping me for all this semantic stuff seems a little ridiculous to me... I've never even had any formal training.

I'm not ripping you. I'm trying patiently to show you how to do it right. This does not replace formal training, but since you're learning on the job, you may as well learn to do it the right way.

So all my work on D6 on this will be totally moot until there is a D7 version of my work?

No. It's a good starting point, and when D7 is done you'll already have the D6 version, so that step will be quick. I don't think D7 is that much different for this issue, and it shouldn't matter much which way you port.

#10 and #11 crossed. In reply to your #10:

I'm not here to do your work for you, but I'll sacrifice some of my precious spare time to help you do it, and I hope that you'll get better at it over time. I'm making an investment in you.

with the D7 version, Mail Edit will be a required dependency now. So, will this effect my code at all?

No, I don't think so. You will need to enable Mail Editor because it's where the template processing is now done, but you just won't define any Mail Edit custom templates if you don't want to.

Yes, funny indeed.

I'm sure you've heard the expression "scratch your own itch". I have no itch anymore to change anything in Subscriptions for D6.

I have this module working fully for my own purposes, but I'm just trying to manage to get the module to a point where I don't have to apply all my updates to it every time a new release comes out.

That's your itch, so you have to scratch it. If you can convince me that a change is a net improvement (benefit - resource_usage - additional_UI_complexity - additional_support - additional_maintenance) then I'll commit it, but you'll have to provide it pretty much ready-to-be-committed.

If you provide a patch that I can't commit, that creates work for me. My itch is not to get your patch committed but to get you to provide better patches. So that's where I'm scratching. :-)

For every line of code that I commit, I'm already taking on the chore to port it to D8, D9, etc. because it's not practical to put that on you, but I refuse to do the D7 porting. That's part of your cost for getting something committed to the legacy D6 version.

Sorry for the delay, I've been sick in bed. This looks good but I need some more time to test it.

Where exactly is "the 'themes' directory"?

sites/all/themes does not seem to work...

Assuming that I run a site on Garland and with Bluemarine as my administration theme, then where do I put the custom default template file?

Well, and if I have no custom theme?

Or if I have one but I'm using a built-in administration theme?

This may not be as easy as we thought it would be...

Exactly, Garland for example. But Garland is under /themes — we cannot put a file there, because that's part of Drupal.

You're missing several points. Let's try to nail down the first one.

Garland normally lives under themes, not under sites/all/themes. That is a fundamental difference.

It may well work if you put the file in themes/garland, but you're not supposed to put things there, and we are good citizens and we do not promote nasty habits.

The same applies to all themes that come packaged with Drupal (and thus live under themes).

Does that make sense?

That makes sense to a degree, but I don't see it working in the real world.

Lets just state that it can only be used with custom-added themes then.

No way! That'd be a support nightmare. The environment that I can support without killing myself is plain-vanilla Drupal. The "real world" is way too diverse to make any promises. If there's a problem in a "real-world" installation and it's working fine in plain-vanilla Drupal, then I'm mostly out of the loop, because I don't need to debug the "real-world" installation. Now, to advertize a feature that "does not work on plain-vanilla Drupal, but only in the real world, but only if you have a custom theme" would be nuts!

In fact, I can see people complaining: "How come when I add the custom template to my default Garland theme folder it doesn't work!?"

Yes, I see that, too.

We have two choices: Make it work with the default Drupal themes (if someone tries it), or don't. You choose.

It needs to work with the default Drupal themes, but I wish there was a solution that did not involve putting the files under the themes directory. That directory branch is off-limits.

I think we should leave it working no matter which theme folder the template is in, but tell people they should only use the template in their "custom theme" folder...

I don't want to actively block themes, of course.

Being vague about where to put the file is not a solution. It will only cause people to come back and ask for support, as I did. The "custom theme" folder is the folder where custom themes go: sites/all/themes.

We have to be more specific, like

Put it into sites/all/themes/<my_theme>; on an out-of-the-box Drupal installation put it into themes/garland for testing purposes, but be aware that putting files under Drupal directories other than sites is not recommended practice.

... if that works... Is that your understanding of how it should work? Does it?

There I go again letting myself be dragged into discussing partial solutions before we've established the full extent of the problem... That is a very inefficient way of working. Can you afford to work that way?

There you go again -- no need to get condescending. I'm not liking that you rip me for the way I work.

That wasn't my intention. I'm sorry if it came through like that.

In #28 I tried to start laying out the problem. I did say there was more to it, but when in response to my question "Does that make sense" you came back with "That makes sense to a degree" and started proposing solutions, I made the mistake of letting myself be dragged into discussing solutions.

I'm not asking you to read my mind but to allow me to fully explain the problem before you cover me with solution proposals.

I did try to let you have it all in #20 and #22, but this didn't work. Then I tried a fresh start in #28, but it only made sense "to a degree". There's no use in talking about solutions as long as we don't have a full common understanding of what the problem is.

You're eager to work on this all day and night, but I only have a few minutes on some days.

You want to get it in quickly, but I'll be stuck with it for years — I'm afraid you'll have to slow down to my pace.

Is there anything in #28 that does not make perfect sense?

No solutions, please!

Great!

There's a feature called admin theme. Do you know that feature?

Admin theme is a core feature and we need to support that.

This means we can have two different themes active at the same time.

Do you have an idea whether the user theme or the admin theme is active during cron, "active" as in $theme_path?
Since you're sending HTML mail, I assume you're getting the correct $theme_path in cron, right? But does it still work if you enable an admin theme that's different from the user theme?

Does it make a difference how cron is run (cron.php, from the status page link)?
(In D7 there's a built-in poormanscron which may behave in yet another way.)

In Mail Editor (where we'd like to have the custom default templates available, too) it'll surely be the admin theme that's active, because Mail Editor is part of the admin interface.

Does this make sense and do you agree that we need to support the admin theme feature (including the potential issues above and maybe others)? Am I missing something?

No solutions, please!

Yes, $GLOBALS['theme_key'], sorry.

drupal_get_path('theme', $GLOBALS['theme_key']) is a nifty way to get around having to know whether the theme is custom or built-in, but if we were to recommend copying the built-in theme to sites/all/themes, can we be sure that Drupal will then use the copy? (NOT looking for solutions yet, just noting down potential problems that we must not forget.) Can you post pointers to documentation where this is defined/explained?

I could be wrong about the $GLOBALS['theme_key'] having just the default theme set in it, though. I couldn't find any definition for it other than "It holds the active theme" in it.

A quick check shows that you are indeed wrong. On my development installation I have Devel module enabled and its Execute PHP block in the footer of every page. Running
dpm($GLOBALS['theme_key']);
on user pages gives me "garland", and running it on the admin/build page gives me "bluemarine". This corresponds to "the active theme" for the current page, and can certainly be different from the "default theme".

Please find out what the situation is in cron.

I know nothing about the Mail Editor module other than it never works for me.

Ouch! That means we live in a different universe. If you had said it worked fine for you, it wouldn't necessarily mean that we're closer together, but saying it never worked for you ascertains a non-negligible minimal distance. My target audience are those for whom ME works (the ME+ types). I have no way of knowing how large the ME- group is, but I sure hope we're working on a feature that will work for the ME+ group as well!

This realization reduces the significance of your testing. The ME+ and ME- environments are obviously different, and what works in one doesn't necesserily work in the other and vice versa! Why does ME not work for you? If we could understand what the problem is, we could gain some insight (and maybe even fix it).

One thing is clear: $GLOBALS['theme_key'] returns the name of the admin theme on the admin/build/mail-edit/subscriptions_content_node-nid/en page, and not the name of the user theme, so when Mail Editor requests the default templates (through Subscriptions), it will not get the custom ones, based on $GLOBALS['theme_key']. Do you agree that this is a problem?

More food for thought: you probably saw it coming by now — what do we do if the admin enables more than one theme? Then every user will have a different $GLOBALS['theme_key'] value... We could try to say that we just don't support that, but having a partial feature is dissatisfying. Kind of like being "98% pregnant". There should be no no-go zones between core and Subscriptions. Do you agree with that?

sites/all/libraries/subscriptions could be a valid alternative, but is there a supported way of finding that directory? A way that will also work if the site does not reside in sites/all but in sites/mysite or in sites/mysite/special?

Let's continue investigating the problem space before jumping to solutions. Does all this make sense to you? Do you see other points that we need to keep in mind?

Take your time with your reply, I probably won't be able to reply in the next two days.

Can we not try to look into why you're hating Mail Editor so much?

ME takes the default templates as starting point for fine-grained customizations, e.g. per content type. This would go well with custom default templates...

Oh, well, at least we've had a lively discusssion...

That's in the context of efforts to send HTML mail, right, not normal text mail?