This follows on from http://drupal.org/node/62209 I have created a new issue as it's a rewrite of the module. This rewrite, I bleieve, addresses all the issues from #62209

The attached emailpage.module is an update on my previous version. It includes :-

  • zoo33's comments regarding non translation strings within t() functions
  • Added in code to check for future additional default values in the place holder class
  • Suppress link if it leads to "access denied" page (a pet hate of mine!)

    Please test and report any issues regarding it here.

    Regards
    AjK

    CommentFileSizeAuthor
    #22 emailpage-module-060623b.patch1.24 KBzoo33
    #21 emailpage-module-060623.patch1.28 KBzoo33
    #15 emailpage_4.module16.18 KBAjK
    #12 emailpage_3.module_patch_2927 byteszoo33
    #9 emailpage_3.module_patch_1106 bytesAjK
    #7 emailpage_3.module.patch_07.88 KBAjK
    #5 emailpage_3-zoo33.patch6.14 KBzoo33
    emailpage_3.module16.56 KBAjK

    Comments

    deekayen’s picture

    deekayen commented

    I know you were counting on me eventually commiting this to CVS, but as of today, I'm locked out. Only the lead maintainer can commit to projects now thanks to a new "feature".

    deekayen’s picture

    deekayen commented

    That's an interesting urlfilter bug, but the feature link was http://lists.drupal.org/pipermail/development/2006-May/016096.html

    AjK’s picture

    AjK commented

    deekayen, I'll try and contact the maintainer regarding either a) getting him to commit when this is ready or b) taking it off his hands completely.

    As for the urlfilter bug, I'm not sure how that relates to the emailpage work I'm doing / have done ??

    regards
    --AjK

    deekayen’s picture

    deekayen commented

    The urlfilter bug doesn't. I just thought it was interesting that urlfilter stripped out the href of my a tag when it was in quotes for what was supposed to be a link for "feature".

    zoo33’s picture

    zoo33 commented
    StatusFileSize
    new emailpage_3-zoo33.patch6.14 KB

    I did another code review, without actually testing it... :)

    I made a few changes and ran a diff (see attached patch) – thought it was easier than commenting on everything in detail. See what you think and use whatever changes you think are worthwile. Some things need explaining though I think:

    1) There were no t() calls used for the strings in the default settings class. (Yes, I'm a bit obsessed with those...) The problem is, if we just add t() functions in the var declarations they will be executed much to early. So I added a constructor method which assigns the variables when the object is constructed.

    2) I removed the <center> tag because I think this is something that should be done with CSS for those who want it. I also changed the <b> tags to <strong> which is more compliant with xhtml. And i think that p tags have to have a separate start and end tag, so i changed <p/> to <br/>. (Maybe this chunk of html should have it's own theme function so that people can change it?)

    3) I added a form field for the sender's name which is only shown if the user isn't logged in. Otherwise the email would read "Sitename.com thought you'd like to see this" which could seem a bit unpersonal.

    4) Then there is the URL that gets sent in the email and that the user gets redirected to after sending it. drupal_goto() does'n require you to deal with base_path, clean urls etc (I'm almost certain of it) so I removed those. And there is an url() function which you can use to build the URL in the email message. This will probably solve lanesharon's problems with path aliases not working.

    5) In _emailpage_local_settings() I changed two variable names to make it easier to understand.

    Then I have another issue which involves more work. Sounds fun? ;)

    I think the form handling in _emailpage_main() is not all done in a preferred way according to Drupal's form api. I checked this page and I think you're actually not supposed to use $_POST explicitly, but rather through api functions. In this case, there should be an email_main_submit() function which recieves the submitted form data. Would you like take a look at that or do you want me to do it?

    AjK’s picture

    AjK commented

    zoo33,

    My thoughts....

    1) Yes, I to am a bit of a t() hound, but I specifically left them out after the initial set-up simply because from that point on the site admin can change them him/her self. I assume they would use their native tongue to do that and therefore running them through t() wouldn't seem appropiate.

    2) remove , yup, good idea, will take on-board.

    3) With regard to adding info that ends up in email, I prefer "the minimum". The more you allow through the more you have to check for abuse/attacks. If a user is logged in, we have their actual user name which is what I'd prefer. But for Anon users' I don;t think it's a good idea to choose any arbitary name they like. So if the admin allows anon posting that's what I think the email should be, anon and not "pretend" to be someone. So, I'd rather leave this as is (I'm open to argument though, so please, comments).

    4) url() OK, I'll look at this as it makes sense.

    5) variable names: I'll look at this too. Anything that makes code clearer is a good thing, thx.

    As for emailpage_main_submit() I'll follow your link and redo this. I'd obviously prefer to always use the recommended route in any API. Using $_POST was somewhat lazy of me following previous examples and not looking at the API in more details.

    Thx for the comments, I expect I'll produce a patch soon.

    Regards
    --AjK

    AjK’s picture

    AjK commented
    StatusFileSize
    new emailpage_3.module.patch_07.88 KB

    Here's the patch based on emailpage_3.module and zoo33's comments and my reply.

    Regards
    --AjK

    zoo33’s picture

    zoo33 commented

    1) In my mind I see the case where someone installs the module and then immediately imports a translation. He/she would then perhaps expect everything to be translated. But I agree, it's not an important issue as the text can easily be changed.

    3) Good point. If anonymous users should give any information, then maybe it's their validated e-mail address... Or maybe we should actually replace %someone with t('Someone') when the user is anonymous. That way it will be a person, 'someone', who recommends the page, not the web site itself.

    Great, thanks for putting so much work into this.

    AjK’s picture

    AjK commented
    StatusFileSize
    new emailpage_3.module_patch_1106 bytes

    3) I like the idea of t('Someone') and the patch attached does this (after applying the previous patch).

    Regards
    --AjK

    lanesharon’s picture

    lanesharon commented

    Can I assume that the suppression of 'access denied' will keep emailpage links from showing up in the my Google sitemap? Downloaded the lates (3) and seems to be chugging along. Thanks!

    AjK’s picture

    AjK commented

    lanesharon, not entirely sure what you mean here. However, the way "suppress link" works is simple. If the link leads to a page that I know will give an "access denied" message if the user follows it then the link is omitted (suppressed) from the node. It's a "bug bear" of mine. I'm not keen on sites that give me lots of jucie looking interesting links only to find that clicking on them leads me to another page that says I have no access. If that's the case, why give me the link? (for flat html not alot you can do about that, but if it's within your power to "know" access will be denied then I think best behaviour is not to show the link).

    Likewise, there is an admin setting "suppress link if flooded". If this is set on then we suppress the link if the user reaches the flood control limit. Flood control is still in action (for those that hand type in the URL) just again, don't see any point in displaying a disfunctional link if you know in advance that's the case. But that can be switched off. A site admin might link to continue displaying the link even if it leads to the warning message when flood control is reached.

    Hope that helps

    best regards
    --AjK

    zoo33’s picture

    zoo33 commented
    StatusFileSize
    new emailpage_3.module_patch_2927 bytes

    Well, i tested it quickly and it seems to work as expected.
    I came up with a few small changes though, see the attached patch.

    AjK’s picture

    AjK commented

    zoo33,

    Looks good, I'll make those amendments and resupply module. How much testing does these need before I approach the project maintainer about a commit?

    fwiw, I have it running on several sites, one of which is busy and it's working a charm for me too :)

    best regards
    --AjK

    zoo33’s picture

    zoo33 commented

    I think it might be good to involve the maintainer sooner rather than later. No use testing this too much until we know if he'll want to rework something before commiting. Or maybe he'd like to give deekayen or you commit priviliges.

    AjK’s picture

    AjK commented
    StatusFileSize
    new emailpage_4.module16.18 KB

    Here's the latest module taking into account zoo33's patch above.

    I have used the drupal conract system to send a message to the project maintainer about where to go next with this issue

    best regards
    --AjK

    zoo33’s picture

    zoo33 commented

    Shouldn't the title of the submission page be "email this page" and not "emailpage this page"?

    Line 74 (in my copy at least):
    'title' => t('email this page'),

    Any news?
    If there is no reply, then perhaps send a message to the maintenance list and ask for advice. Maybe emailpage needs a new maintainer. peterjohnhartman doesn't seem to have been active on drupal.org for a couple of months.

    AjK’s picture

    AjK commented

    Zoo33,

    Patch for that below. As for Peter Hartman, I've had an email from him
    stating that he'd have a look this week.

    best regards
    --AjK

    --- emailpage_5.module.ajk      Thu May 18 14:13:34 2006
+++ emailpage.module    Thu May 18 14:13:47 2006
@@ -71,7 +71,7 @@ function emailpage_menu($may_cache) {
   if($may_cache) {
     $items[] = array(
       'path' => 'emailpage',
-      'title' => t('emailpage this page'),
+      'title' => t('email this page'),
       'access' => user_access('access emailpage'),
       'type' => MENU_CALLBACK,
       'callback' => '_emailpage_main'
    budda’s picture

    budda
    Primary language English
    Location Cheshire
    commented

    What about adding the "display email link" as a setting on the ?q=admin/settings/content-types nodetype pages?

    At least then all the node type specific settings are in one place.

    budda’s picture

    budda
    Primary language English
    Location Cheshire
    commented

    New module seems much nicer, but i don't want user emailing pages to know the author of the node. That's why i turned off show post info in the theme settings. Maybe emailpage module could also respect these settings when deciding what to show on the ?q=emailpage/xx page?

    zoo33’s picture

    zoo33 commented

    Good points, budda. Especially the first one. The second one (showing or hiding author) could also be a setting in ?q=admin/settings/emailpage

    zoo33’s picture

    zoo33 commented
    StatusFileSize
    new emailpage-module-060623.patch1.28 KB

    Another bug smashed.

    The email validation was not working properly (try to send a page to an invalid address and you'll see for yourself). I moved it to a _validate() function which is the preferred way of doing it.

    I also moved a variable assignment ($name = $user->name;) to a more logical place. Oh, and the ?> at the end of the file should be omitted if I'm not misinformed.

    zoo33’s picture

    zoo33 commented
    StatusFileSize
    new emailpage-module-060623b.patch1.24 KB

    Well I don't know what happened with my last patch but there were two lines that shouldn't have been there. Try this one instead. If it doesn't apply, I'm sorry – please check the changes manually.

    AjK’s picture

    AjK commented

    zoo33, looks good to me. But I'd love to know where you heard about ?> not being required at the end of a module? I've never heard of any php script that gets include_once()'ed without a closing ?> to match the opening <?php ?

    best regards
    --AjK

    deekayen’s picture

    deekayen commented
    Title: New/re-write of email this page mdoule » New/re-write of email this page module

    Leaving off the ending bracket is legitimate. If you look at Drupal core files, most don't have the ending bracket anymore to prevent extra space from sneaking into the output and breaking HTTP header output.

    zoo33’s picture

    zoo33 commented

    I found it here:

    Note that the final ?> should be omitted from all code files--modules, includes, etc. The closing delimiter is optional, and removing it helps prevent unwanted white space at the end of files which can cause problems elsewhere in the system.

    zoo33’s picture

    zoo33 commented

    Well, this module was downloaded 1122 times in June. It would certainly be nice if it was functional. Still no news on a commit? I sent an email to the maintainer, but I didn't get a reply.

    budda’s picture

    budda
    Primary language English
    Location Cheshire
    commented

    Use the 'forward' module - it works well in 4.7 and the author is active when committing patches to fix bugs :)

    darren oh’s picture

    darren oh
    Primary language English
    Location Lakeland, Florida
    commented

    Marked issue 70342 as a duplicate of this issue.

    darren oh’s picture

    darren oh
    Primary language English
    Location Lakeland, Florida
    commented
    Status: Needs review » Fixed

    The Drupal 4.7 branch now uses the code from this issue.

    darren oh’s picture

    darren oh
    Primary language English
    Location Lakeland, Florida
    commented

    By the way, the fix was made in CVS commit 45506.

    Anonymous’s picture

    (not verified) commented
    Status: Fixed » Closed (fixed)