Huge FAPI, namespace, logic, kitchen sink cleanup

This is awesome RobRoy. Thanks for your work!

Thanks for all your hard work on this - most impressive patch will give it a test now.

Not sure if this is a feature request or a bug but I'm still struggling with how to allow users individual newsletters which don't interfere with each other. There's a 'edit own newsletter' privilege which seems to suggest they should be restricted to their own but when creating they're shown all the names so allowing them to create issues on each others newsletters which doesn't seem right?

Thanks for all your work on this.
I can't comment on the quality of the code, but I have had a chance to do some initial testing.

1 error I am getting at the moment, from the 'Manage subscriptions' link under 'My Account':
Fatal error: Call to undefined function simplenews_subscription_manager() in /pathtodrupal/modules/contrib/simplenews/simplenews.module on line 512

I like the way that it now works with mimemail.module and that you've introduced theming for the various elements.
I've not had chance to test the theming yet.

One thing that is a big hang up for me is the relative links for images in outgoing mail. With theming a base url can be added, but it would be nice if the absolute url could be added automatically so it 'works out of the box'. I can't think of an instance when this wouldn't be needed other than if the images were embedded.

Mark

I'll give the new patch a test out.

That's good that mimemail handles the absolute urls for images, and I like that multi-part sending option. I'll give this another tryout, and include some theming tests.

BTW is it usual for mimemail to encode the entire html portion as base64? I thought this was only used for attached images? It's quite hard to see what's going on in the received email code.

Mark

you can add support for absolute urls by changing in simplenews.module

    $pattern = '@(<a href=)"(.\S+?)"([^>]*>)@ei';
    $node->body = preg_replace($pattern, "'\\1' . '\"' . _sn_mail_url('\\2') . '\"' . '\\3'", $node->body); 

to

    $pattern = '@(<a href=)"(.\S+?)"([^>]*>)@ei';
    $node->body = preg_replace($pattern, "'\\1' . '\"' . _sn_mail_url('\\2') . '\"' . '\\3'", $node->body); 
    $pattern = '@(src=)"(.\S+?)"([^>]*>)@ei';
    $node->body = preg_replace($pattern, "'\\1' . '\"' . _sn_mail_url('\\2') . '\"' . '\\3'", $node->body);

I did a little bit of testing, but note that I didn't test pre-patch, that is, I updated to the latest 4.7 VPS and applied the patch.

Nothing obviously broken in sending out newsletters, first a bunch of plain text newsletters, then a couple of HTML newsletters using Mimemail, with the only HTML being an <img> tag pointing to an external image.

Now that Drupal 5 is right around the corner, do you think it's a bad idea to abandon the 4.7 version?

Yes, it's a bad idea to abandon 4.7. There are many of us who will be using 4.7 for months to come, and these are fixes that have been needed for a while.

Drupal 5 may be "just around the corner", but the contributed modules are months away from being updated and debugged so that everything works smoothly in 5.

Of course, by then some people will be wanting to abandon 5 before all of the pieces are in place so they can jump to the latest new thing. Such a vicious cycle which can make real-world implementation quite difficult.

peace,
blue

+1 for your great patch!

I was not able to test it on my system, because I don't sit on my testing machine.
but I reviewed it visually...

the only thing I found is this:

$footer = "\n\n-- \n". t('Subscribe link:') .' '. url('newsletter/confirm/add/'. $hash, NULL, NULL, TRUE);

it is better if you do this:

$footer = "\n\n-- \n". t('Subscribe link: %url', array('%url' => url('newsletter/confirm/add/'. $hash, NULL, NULL, TRUE)));

the same for

+        $footer = "\n\n-- \n". t('Unsubscribe link:') .' '. url('newsletter/confirm/remove/'. $hash, NULL, NULL, TRUE);

and

+    $footer = "\n\n-- \n". t('Visit our site:') .' '. url('', NULL, NULL, TRUE);

I've tested the patch and eveything is working smoothly for me.

I've still not got the absolute urls working for images.

unionweb suggested some code to replace, but that doesn't appear in the patched module code.
Can this be included as it doesn't look like mimemail does this.

For now I'm hoping to add the preg_replace pattern to template.php and overide the theme:

<?php
function phptemplate_simplenews_newsletter($node, $tid) {
  $term = taxonomy_get_term($tid);
  $node->subject = '['. $term->name .'] '. $node->title;
  $node->body = '<h2>'. $node->title ."</h2>\n". $node->body;
  return $node;
}

Can somebody tell me how to apply this?

Rob, I also think the code should include a header theme - maybe the site logo by default.
Great work!

Mark

Rob,

I've had a quick look at mimemail project page and it says:
# If the HTML has embedded graphics, these graphics are MIME-encoded and included as a message attachment.
# Adopts your site's style by automatically including your theme's stylesheet files in a themeable HTML message format

I'm not sure that including images as attachments is the best way to go - are you?

Also there doesn't seem to be much in the way of my sites styles coming through with the mail. This isn't a problem for me as I'd rather send a restricted set of styles anyway. I'd hoped to include this in the theming functions.

I'm sure these pointers come under 'feature requests', which I know you're not looking at with this patch.

Mark

I've now applied this patch on a live/test server and the images are being included with the mail. The images are sent as attachments as intended with mimemail.module. I'd prefer the images to be referenced off the server, but I guess that's an issue for mimemail.

I've used template.php to overide the css inclusion and send only the code I need:

	function phptemplate_mimemail_message($body) {
	  $output = '<html><head><style type="text/css"><!--' ;
	  // include a stylesheet specifically for the newsletter
	  $css = file_get_contents("$_SERVER[DOCUMENT_ROOT]/modules/contrib/simplenews/newsletter.css");
	  $output .= $css;
	  $output .= '--></style>';
	  $output .= '</head><body id="mimemail-body">';
	  $output .= '<div id="newsletter-header" style="border: 1px solid black;">Site Name</div>';
	  $output .= '<br />'.$body.'</body></html>';
	  // compress output
	  return preg_replace('/\s+|\n|\r|^\s|\s$/',' ',$output);
	}

It's now quite usable for me.

Thanks
Mark

I'm using the patched version on a live site now, and through some recent testing have noticed that the 'Display block message' only works if the user doesn't have 'subscribe to newsletters' access control.

So a message like 'Stay informed on our latest news!' is only displayed for users that can't subscribe. :(

I'm assuming this isn't by design?

keeping it up!

Under the settings for a newsletter there is an option for HTML to text conversion. The options don't work as the plain text version (within multi-part html and plain text) always sends with numbered links at the footer.

Looking forward to giving this a spin in the 4.7.x-2.x-dev branch :)

Yes I'll post issues against the new version - will I need to get this from CVS? I've not done much testing and issue posting before.

Budda, good to see you here! I'm using your returnpath module but it doesn't seem to be working with mimemail, now used with this version of simplenews. (but that's a separate issue I guess).