HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.

Marking on "words" not strings within words

This discussion is related to the Discussion Marker addon.
rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one"NY ✭✭✭

Am I wrong in that the "Discussion Marker" plugin marks the discussion even if the term is within another word? If so it is pretty unusable for us.

Is there a way to make it look for words (which of course could be delimited by spaces,commas, etc.).?

Comments

  • hgtonighthgtonight ∞ · New Moderator

    According to the source here: https://github.com/bleistivt/DiscussionMarker/blob/27544291eaef7062ada00ce2942a22f79520ae3b/class.discussionmarker.plugin.php#L67

    The regex uses the word boundry character \b on the beginning and end so it should not match in words withing words.

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

  • @hgtonight said:
    According to the source here: https://github.com/bleistivt/DiscussionMarker/blob/27544291eaef7062ada00ce2942a22f79520ae3b/class.discussionmarker.plugin.php#L67

    The regex uses the word boundry character \b on the beginning and end so it should not match in words withing words.

    Yes, it does :D

    ...in the new version I just uploaded.

    @rbrahmson You can try out if the new version works for you

    hgtonightR_Jrbrahmsonvrijvlinder
  • hgtonighthgtonight ∞ · New Moderator

    @Bleistivt said:
    rbrahmson You can try out if the new version works for you

    Should have checked that commit time. :chuffed:

    Search first

    Check out the Documentation! We are always looking for new content and pull requests.

    Click on insightful, awesome, and funny reactions to thank community volunteers for their valuable posts.

    rbrahmson
  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

    To @hgtonight and @Bleistivt: Thank you -- this new version corrects the issue.

  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

    So a bit of confession -- I am not a programmer, not even an aspiring one, and I wanted the discussion marker plugin to also search the body of the discussion, not just the title. I decided to take a look at the 1.7 source and try something out- sort of a gamble. If it worked I figured I'd put it on this discussion as a suggestion. Well, the gamble worked for a very short while - until I discovered the substring issues which were fixed in Ver 1.8 but broke my code (if you can even call my amateurish changes code).

    Here is what I did on Version 1.7 (changed marked with //PA):

        protected function DisplayMarker($Sender) {
                $Discussion = $Sender->EventArguments['Discussion']; //Original source
                $title = val('Name', $Discussion);                                    //Original source
            $thebody = val('Body', $Discussion);                               //PA body contents
    

    and later on

    $pos = stripos($title, $Marker);                     //Original source 
    $posbody = stripos($thebody, $Marker);       //PA Search for body contents as well
    

    and the next line changed to:
    if (is_numeric($pos) || is_numeric($posbody)) {

    These are all my changes to 1.7. I didn't expect it to work but it did. But now, these won't work any more because Version 1.8 changed the function...

    Can someone implement this feature (possibly as an admin set option - performance was fine for my small forum) on the new Ver 1.8 (and I'm sure in a more professional way than I attempted).

  • I "inherited" this plugin from peregrine and had not changed anything until now, so when I applied the fix I also adapted it to the new coding standard and shortened the function a little.

    With your extended functionality (wich doesn't look amateurish at all), it would look like that from line 63:

        $title = val('Name', $args['Discussion']);
        $body = val('Body', $args['Discussion']);
    
        $markerArray = preg_split('/ *, */', C('Plugins.DiscussionMarker.WordList'));
        foreach ($markerArray as $marker) {
            if (!preg_match('/\b'.$marker.'\b/', $title.' '.$body)) {
                continue;
            }
    
    rbrahmsonvrijvlinder
  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

    Thanks @Bleistivt , before I test it just a thought - instead of searching twice in both the body and the title, would it be more efficient to concatenate the two (with a period in between to prevent inadvertent string matching) and perform one search on the concatenation?

    (I do remember something from a PL/1 coding class thirty years ago;-)

  • I don't know, probably. I wrote it like that, because it's shorter and that is all that counts.

    Minimal performance improvements are neglible if you can achieve cleaner code that is easier to maintain.
    Most of the plugins execution time is probably spent dispatching to the event handlers or some other PHP magic, anyway.

    Note that it loops over all the markers, so the number of markers you have has a direct influence on the speed.

    I did a small test with 1000 markers. I didn't notice a speed difference.

    rbrahmsonvrijvlinderhgtonight
  • rbrahmsonrbrahmson "You may say I'm a dreamer / But I'm not the only one" NY ✭✭✭

    I want to thank @hgtonight and @Bleistivt for helping in directing, correcting, and updating this plugin. I took the most recent version 1.8.1, added @Bleistivt suggestion to modify the plugin to search the body of the discussion as well, and then I made a change to the options screen adding that body search as an admin option. Thus the update may be useful to all. Here are the changes:

    To discussionmarket-settings.php:

            echo $this->Form->CheckBox('Plugins.DiscussionMarker.GroupLabels', "");
            echo "</td>";
            echo "</tr>";
            echo "<td>";
            echo T("Search discussion body as well (otherwise only title is searched)");
            echo "</td>";
            echo "<td>";
            echo $this->Form->CheckBox('Plugins.DiscussionMarker.SearchBody', "");
            echo "</td>";
            echo "</tr>";
    

    To the class.discussionmarker.plugin.php:

            protected function displayMarker($sender, $args) {
            $title = val('Name', $args['Discussion']);
            $body = val('Body', $args['Discussion']); //PA 
            $fulltext = $title.' '.$body;             //PA
    
            if (c('Plugins.DiscussionMarker.SearchBody', false)) {
    
               $fulltext = $title.' '.$body;      //PA
            } else {
               $fulltext = $title;                //PA  
            }
    
            $markerArray = preg_split('/ *, */', c('Plugins.DiscussionMarker.WordList'));
    
            foreach ($markerArray as $marker) {
                if (!preg_match('/\b'.preg_quote($marker, '/').'\b/', $fulltext)) {
                    continue;
    

    Hope this will help someone and perhaps the plugin author (Peregrine?) may find it useful to add to the next version.

    Bleistivthgtonight
  • businessdadbusinessdad Stealth contributor MVP

    @Bleistivt said:
    Yes, it does :D
    ...in the new version I just uploaded.

    A clbuttic mistake.

    hgtonightrbrahmson
Sign In or Register to comment.