HackerOne users: Testing against this community violates our program's Terms of Service and will result in your bounty being denied.
Please upgrade here. These earlier versions are no longer being updated and have security issues.

Messages AfterFormatBody

Hey guys,

So for comments & discussions there's a nice AfterCommentFormat event that's fired (in helper_functions.php) after the comment is formatted and before it's outputted to the client, but for private messages no event is fired after formatting but before outputting:


Is there any reason for the difference in behaviour?

For my own version I adapted messages.php to this:


But I kinda dislike having to edit core files. Is there a cleaner way to do this?

Comments

  • submit a pull request.

    grep is your friend.

  • Before I do that: Would it be better to change

                     echo Gdn_Format::to($Message->Body, $Format);
    

    To

                     echo formatBody($Message);
    

    Or

                    $this->fireEvent('BeforeConversationMessageBody');
                    echo Gdn_Format::to($Message->Body, $Format);
                    $this->EventArguments['Message'] = &$Message;
                    $this->fireEvent('AfterConversationMessageBody');
    

    To:

    $this->fireEvent('BeforeConversationMessageBody');
                    $this->EventArguments['Message'] = &$Message;
                    $Message->FormatBody=Gdn_Format::to($Message->Body, $Format);
                    $this->fireEvent('AfterConversationMessageBodyFormat');
                    echo $Message->FormatBody;
                    $this->fireEvent('AfterConversationMessageBody');
    

    I'm personally in favour of the former.
    If people have a custom formatBody function for discussions and comments, it's also directly applied to PMs in this way.

  • Scratch my previous post. Since messages.php is in the conversation application and helper_functions.php is in the vanilla application (and V comes later alphabetically than C) the FormatBody function doesn't exist yet when it tries to format a PM.

    So second option it is!

  • R_JR_J Ex-Fanboy Munich Moderator

    You haven't told us what you are trying to achieve, that would help in most cases ;)

    As far as I can tell, you would like to take influence on what is written to screen. While in discussions and comments the output isn't directly echoed, the message body is. If you want to do some extra transformation to the text, you have options nevertheless. One thing which is possible quite often is to manipulate the data before it is written to screen, so that nothing happens at all and at the next event, do what you like.

    See below for an example:

        public function messagesController_beforeConversationMessageBody_handler($sender, $args) {
            $args['Message']->BodyCopy = $args['Message']->Body;
            $args['Message']->Body = '';
        }
    
        public function messagesController_afterConversationMessageBody_handler($sender, $args) {
            // Reset message body.
            $args['Message']->Body = $args['Message']->BodyCopy;
            $args['Message']->BodyCopy = null;
    
            // Get format.
            if (empty($args['Message']->Format)) {
                $format = 'Display';
            } else {
                $format = $args['Message']->Format;
            }
    
            // Format message.
            $formattedMessage = Gdn_Format::to($args['Message']->Body, $format);
    
            // Now do your own transformations.
            // ...
    
            // Print result to screen.
            echo $formattedMessage;
        }
    




    But to be honest: that is a cheap trick and here you have a much better alternative. You can create custom formatter functions. So if the message would have the format "CustomWysiwyg", you would have to define a function formatCustomWysiwyg which would handle that function. Now all you have to do is to change the format of the messages.

    Before the view is called, the messages controller fires an event "beforeMessages". Here you would be able to change the format:

        public function base_beforeMessages_handler($sender, $args) {
            while ($message = $args['MessageData']->nextRow()) {
                $message->Format = 'Custom'.$message->Format;
            }
        }
    

    You would have to make sure that you have a custom function for all formats that there are in the database: a formatCustomBBCode, formatCustomWysiwyg, etc.

    That function could look like that:

    function formatCustomWysiwyg($mixed) {
        $result = Gdn_Format::to($mixed, 'Wysiwyg');
        $result = strtoupper($mixed);
    }
    

    In this case it would first do all the stuff that "Wysiwyg" format does and afterwards convert everything(!) to upper case.




    Whatever you do and whatever you like to achieve here, I do not think you need to change any of Vanillas core files for that.


  • R_JR_J Ex-Fanboy Munich Moderator

    @Caylus said:
    Scratch my previous post. Since messages.php is in the conversation application and helper_functions.php is in the vanilla application (and V comes later alphabetically than C) the FormatBody function doesn't exist yet when it tries to format a PM.

    So second option it is!

    If you need a function from a helper_functions.php you can include it like that:

    include $this->fetchViewLocation('helper_functions', 'discussion', 'vanilla');.

    I guess you see the pattern: it is view-name, controller-name, application-name. So if you would like to use a function from such a helper file, simply include it! B)


  • CaylusCaylus ✭✭
    edited November 2016

    Oof, I'll try to explain what I'd like to do in a non-TL;DR way.

    Basically, I've written some forum games plugins. Diceroller, mastermind, hangman, a sketching game etc.
    As an example, you could roll a few dice with [roll]7d6b3+2^(1d4)[/roll] (I realize there are two diceroller plugins on here already, but both the old diceroller plugins don't work anymore with Vanilla >= 2.2.1, and could only do very basic arithmatic anyway).

    That'll output something some HTML. With the HTMLAwed plugin I can set which HTML classes are allowed. Cool! Except now it doesn't work with the BBCode formatter. So I have to make an exception for that. And so on and so on.
    Not to mention that between Vanilla 2.2.1 the BBCode formatter switched between being a plugin and a core file with slightly different event names. So to accomodate as many Vanilla versions as possible, I'd have to catch both events.

    ...Which was getting too complicated for me to be honest to catch all the cases of different formatters.

    So what I do now for comments is simply let it all be formatted, and then change the HTML of the formatted comment (taking extra care not to fuck up with XSS vulnerabilities of course).
    This makes my plugin work for whatever format a forum might require comments to be in.

    I appreciate both your solutions, but if I implement them in my plugin they both seem to have the potential to interfere with other plugins in major ways. Other plugins might try to change the message body as well and find it empty, or they might be depending on the format information to perform their function and not realize customwyswyg is basically the same as wyswyg. Suddenly it's really important in what order plugins work on the private message, which is undesirable IMHO.

  • R_JR_J Ex-Fanboy Munich Moderator

    I've never used a dice roller. The difficulty here is that a) the number shouldn't change any more and b) no one should be able to "fake" a dice roll, correct?

    Here's what I would do...

    1.Determine patterns for
    1.1 the dice [dice]youknowthatbetterthanme[/dice]
    1.2 the dice result [dice=result]youknowthatbetterthanme[/dice]
    1.3 the html format `whatever

    1. On save (discussion/comment/message/activity/activity comment) do a regex search for
      2.1 a faked dice result: 1.2 and insert something unneeded to it like so [<i></i>dice...
      2.2 a faked html format and change the class to "FakeDiceResult"
      2.3 add the random value to the correct dice pattern...

    Wait, there is a problem: if users are allowed to edit their posts, the dice roll must keep the value, so the value must be stored separatly from the body. You would have to find a way to store those values.

    At this point I would have a look at how it has been implemented by others... o.O


  • CaylusCaylus ✭✭
    edited November 2016

    Ah, I was unclear. The DiceRoller is not the problem here.

    That's done.

    Works perfect with comments and discussions.

    Is stored in the database, if people try to remove it by editing, it shows that it misses a roll in the post in bold red text, if people try adding old rolls to a new post, they're a different colour, so it's easy to see what the new rolls are and what the old rolls are.

    For example:

    [roll](1d6)d8+3*2[/roll]
    

    is parsed on save, and then a reference to the roll stored inside the comment as:

    [r]21[/r]
    

    (where 21 is the unique ID of the roll).

    Which is translated to this HTML when it's time to display it:

    <div class="DRDiceRoll DRNewroll">
          <p>[(1d6)d8+3*2]: 16</p>
                  <div class="Spoiler">
                         <p class="DRDie">d6:2</p>
                         <p class="DRDie">d8:3</p>
                         <p class="DRDie">d8:7</p>
                    </div>
    </div>
    

    My only problem is that if I try adding this HTML to private messages, it gets formatted by the different formatters differently.

    BBCode uses htmlentities() on the whole thing so no HTML comes through, Wyswyg allows certain HTML classes to pass through, Text also uses htmlentities() on the whole thing.

    That's why if I can bypass the formatter for private messages (**as I can already do within discussions and comments with the existing AfterCommentFormat event **), I wouldn't have to bother with the different formatters at all. (not completely true, I make sure that commands within code blocks get ignored and code blocks differ slightly per formatter, but that's really a minor thing).

    It's quite possible that bypassing the formatter is a bad idea, but in that case I wonder why it's possible to bypass it with discussions and comments, if it's a bad idea for private messages.

  • R_JR_J Ex-Fanboy Munich Moderator

    If "on save" the dice is replaced with a [r]diceID[/r] you could leave the output as it is and do it like that:

        public function base_beforeCommentBody_handler($sender, $args) {
            if (isset($args['Comment'])) {
                // Convert comments.
                $args['Comment']->Body = $this->parseDice(
                    $args['Comment']->Body,
                    $args['Comment']->InsertUserID,
                    $args['Comment']->CommentID,
                    'Comment'
                );
            } else {
                // Convert discussions.
                $args['Discussion']->Body = $this->parseDice(
                    $args['Discussion']->Body,
                    $args['Discussion']->InsertUserID,
                    $args['Discussion']->DiscussionID,
                    'Discussion'
                );
            }
        }
    
        public function base_beforeConversationMessageBody_handler($sender, $args) {
            // Convert messages.
            $args['Message']->Body = $this->parseDice(
                $args['Message']->Body,
                $args['Message']->InsertUserID,
                $args['Message']->MessageID,
                'Message'
            );
        }
    
        private function parseDice($mixed, $userID, $postID, $postType) {
            // Get all [r]number[/r], check if diceID and other IDs fit
            // in order to prevent users from simply using [r]1[/r]
            // in their discussions
            return transformed string
        }
    


  • CaylusCaylus ✭✭
    edited November 2016

    No see, that doesn't work. base_beforeConversationMessageBody_handler is fired béfore the body is formatted. So any HTML you add to $args['Message']->Body is formatted as well.

    If for example the format of your comment/PM is set to BBCode, then your HTML turns from:

    <p>Text</p>
    

    Into:

    &lt;p&gt;Text&lt;/p&gt;
    
  • R_JR_J Ex-Fanboy Munich Moderator

    Yes, I see. In order to take influence you need to either change the format or work with the formatted message. I see no simple way to solve that problem without changing core.


    If you add the format to the event arguments, you would be able to use a custom format function. That approach is elegant since it doesn't change any behavior at all, but empowers developers.

    <div class="Message">
        <?php
        $this->EventArguments['Format'] = &$Format;
        $this->fireEvent('BeforeConversationMessageBody');
        echo Gdn_Format::to($Message->Body, $Format);
        $this->fireEvent('AfterConversationMessageBody');
        ?>
    </div>
    

    I've seen that you made a PR which first formats the body and then fires an event. It would be better to do it like that:

    <div class="Message">
        <?php
        $Message->FormatBody = Gdn_Format::to($Message->Body, $Format);
        $this->fireEvent('BeforeConversationMessageBody');
        echo $Message->FormatBody;
        $this->fireEvent('AfterConversationMessageBody');
        ?>
    </div>
    

    Although there is a theoretical possibility that this might change behavior, it keeps code readable.
    I have a lot(!) of plugins installed and the BeforeConversationMessageBody is not used at all. So this change should be save.


    The line $this->EventArguments['Message'] = &$Message; in this paragraph is totally useless by the way.


    Caylus
  • CaylusCaylus ✭✭
    edited November 2016

    @R_J said:



    I've seen that you made a PR which first formats the body and then fires an event. It would be better to do it like that:


    Although there is a theoretical possibility that this might change behavior, it keeps code readable.
    I have a lot(!) of plugins installed and the BeforeConversationMessageBody is not used at all. So this change should be safe.


    The line $this->EventArguments['Message'] = &$Message; in this paragraph is totally useless by the way.

    Thanks for going over my PR!

    Yeah, I was reasonable sure that line was useless but since it was in the original I didn't know if I was allowed to remove it :P Didn't know if it was a coding standard to improve readability to repeat assignments to EventArguments every time you fire an event, or something.

    Are you sure by the way it's a good idea to do it like you propose?
    IMHO, that might cause problems for developers of plugins later on. Like, if you catch a "base_beforeCommentBody_handler" event, and add to the Body, you expect it to be formatted and then be passed to an AfterCommentBodyFormat event, and finally outputted (which all happens as expected). But if you catch a BeforeConversationMessageBody, and add to the Body, suddenly it doesn't do anything.

    You're sacrificing consistent behaviour for only a very slight improvement in readability. But IDK, I don't have experience with writing open source :P

  • R_JR_J Ex-Fanboy Munich Moderator

    @Caylus said:
    Yeah, I was reasonable sure that line was useless but since it was in the original I didn't know if I was allowed to remove it :P Didn't know if it was a coding standard to improve readability to repeat assignments to EventArguments every time you fire an event, or something.

    No, there is absolutely no reason to keep that line, it is just a relict. Simply remove it and note down that $Message is already passed by reference before. Vanilla has a very detailed source code but repeating such assignments would be a bit to talkative...

    @Caylus said:
    Are you sure by the way it's a good idea to do it like you propose?

    My favorit is to only pass $Format to the event arguments, remember? ;)

    @Caylus said:
    IMHO, that might cause problems for developers of plugins later on. Like, if you catch a "base_beforeCommentBody_handler" event, and add to the Body, you expect it to be formatted and then be passed to an AfterCommentBodyFormat event, and finally outputted (which all happens as expected). But if you catch a BeforeConversationMessageBody, and add to the Body, suddenly it doesn't do anything.

    If a developer assumes anything without checking the source, he will fail anyway. In Vanilla not all events may be named consistently, but if you find an event in a view that is named Before/AfterPartOfSomeOutput the only thing that you can safely expect is that if you echo something in there, it would be in the DOM before/after the named part of the output.

    You always have to check which parameters are passed, you always have to check what would be the correct html markup (some events are fired in ul tags, so you would have to echo li tags) and you always have to check which transformations have been done and will be done if you are planning to use one of the event arguments.

    @Caylus said:
    You're sacrificing consistent behaviour for only a very slight improvement in readability. But IDK, I don't have experience with writing open source :P

    You are right, consistency is very important. If you break something, you need to have a reason for it. And an excuse... ;)
    The reason is, that there are some kind of manipulations impossible by now. So something should be changed.
    The excuse is that there is no usage of BeforeConversationMessageBody in Vanilla or in the addons and - I have searched the about 100 plugins that I have downloaded - there most probably is no plugin that uses this event. So the danger of breaking something is reasonably small.

    By the way: it is not only about readability. Each event fired costs time. And if you fire three events where two would be enough, you should think about possible improvements.


    But the above still is a (small) change and that's why I would prefer to simply add $Format to the event arguments.
    Put that right below the line where the variable $Format is assigned: $this->EventArguments['Format'] = &$Format;
    That would result in no change at all but give you all possibilities.

    Put that method to your plugin:

    public function messagesController_beforeConversationMessageBody_handler($sender, $args) {
        // Change the format that will be used for the message to a custom format.
        $args['Format'] = 'diceRoller';
        // Temporarily store all the values that will be needed to evaluate the dice roll
        $sender->setData('UserID', $args['Message']->InsertUserID);
        $sender->setData('PostID', $args['Message']->MessageID);
        $sender->setData('PostType', 'Message');
        // And don't forget the original format.
        $sender->setData('Format', $args['Message']->Format);
    }
    

    It switches the format to a custom function where you will be able to do everything you have to:
    1. Do the desired formatting
    2. Check for all dice rolls
    3. Validate that the dice roll belongs to this post
    4. Replace valid dice roles with whatever you like

    Put your custom function below the plugin class. It could look similar to this:

    function formatDiceRoller($mixed) {
        // Do original formatting - whatever that may be.
        $formattedBody = Gdn_Format::to(
            $mixed,
            Gdn::Controller()->data('Format')
        );
        // Do some regex magic to get all dice ids of this message
        $diceIDs = [];
        // Get corresponding values from db.
        $diceValues = Gdn::sql()
            ->select('DiceID, Result')
            ->from('DiceRollerResult')
            ->whereIn('DiceID', $diceIDs)
            ->where('UserID', Gdn::Controller()->data('UserID'))
            ->where('PostID', Gdn::Controller()->data('PostID'))
            ->where('PostType', Gdn::Controller()->data('PostType'))
            ->get();
    
        foreach ($diceValues as $dice) {
            $pattern = 'some regex to search the dice '.$dice->DiceID;
            $formattedBody = preg_replace($pattern, $dice->Result, $formattedBody);
        }
    
        return $formattedBody;
    }
    

    See what you can do just by using the possibilities of the framework? And you do not have to really change anything at all. Only add one line (which should find its way into Vanilla after you have made a PR ;) )


  • Hmmmm... how does one change the changes one proposed on Github?

    Because I think I only need to move one line:

                    $this->fireEvent('BeforeConversationMessageBody');
    --> If I move it here you can change the format of the message in BeforeConversationMessageBody, and you don't have to add it as a seperate eventArgument:
                    $Format = empty($Message->Format) ? 'Display' : $Message->Format;
                    echo Gdn_Format::to($Message->Body, $Format);
    

    And delete: $this->EventArguments['Message'] = &$Message;

    Thanks for thinking with me!

  • hgtonighthgtonight ∞ · New Moderator
    edited November 2016

    @Caylus said:
    Hmmmm... how does one change the changes one proposed on Github?

    Because I think I only need to move one line:

                    $this->fireEvent('BeforeConversationMessageBody');
    --> If I move it here you can change the format of the message in BeforeConversationMessageBody, and you don't have to add it as a seperate eventArgument:
                    $Format = empty($Message->Format) ? 'Display' : $Message->Format;
                    echo Gdn_Format::to($Message->Body, $Format);
    

    And delete: $this->EventArguments['Message'] = &$Message;

    Thanks for thinking with me!

    Commit new changes to the branch you used on the PR and the website will reflect that change. In this case, the branch is called 'patch-2'. You can edit it via the the website here: https://github.com/Caylus/vanilla/tree/patch-2

    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.

    R_JCaylus
  • R_JR_J Ex-Fanboy Munich Moderator

    Look at the fireEvents in this view as an example:
    $this->fireEvent('BeforeConversationMessageItem'); is directly before the opening li.Item tag
    $this->fireEvent('AfterConversationMessageDate'); is directly after the span.DateCreated
    $this->fireEvent('BeforeConversationMessageBody'); is directly before the message body
    $this->fireEvent('AfterConversationMessageBody'); is directly after the message body

    See the pattern?

    If you move the event like you plan, you should rename it to SomewhereBeforeConversationMessageBody =)


    I see three possible solutions:
    1) An additional event before $Format is assigned
    2) Assigning the formatted output to a variable which is accessible in an event before that variable is echoed
    3) Provide access to $Format in an event before the body gets formatted

    You've started with a mixture of 1) and 2), now you are thinking of something similar to 1) by not adding an event but moving it to a different place.

    What is wrong with 3)? Adding an element to an array is way more faster than making a call to fireEvent().


    hgtonight
  • R_JR_J Ex-Fanboy Munich Moderator

    By the way: you can still loop through all messages before they are even passed to the view, add an additional field for the "original" format and replace the format with your custom format.

    In the BeforeConversationMessageBody event you can store the needed information (including the OriginalFormat) and use this information in your custom format function.

    So there isn't any change needed at all. It would just make the life of a plugin developer more convenient.

    And since you want to write your plugin now, and not when the next version of Vanilla that includes your PR is released, you should consider doing it that way.


    Caylus
  • @R_J said:
    By the way: you can still loop through all messages before they are even passed to the view, add an additional field for the "original" format and replace the format with your custom format.

    In the BeforeConversationMessageBody event you can store the needed information (including the OriginalFormat) and use this information in your custom format function.

    So there isn't any change needed at all. It would just make the life of a plugin developer more convenient.

    And since you want to write your plugin now, and not when the next version of Vanilla that includes your PR is released, you should consider doing it that way.

    Thanks! Going to try this!

  • This works:


    Nice. Bit of an ugly hack, but at least it works without having to edit core files. Thanks R_J!

    And thanks hgtonight for the github tutorial!

    hgtonight
Sign In or Register to comment.