Uploaded image for project: 'phpBB3'
  1. phpBB3
  2. PHPBB3-13815

Event parameters in posting do not affect behavior as expected

    Details

    • Type: Bug
    • Status: Unverified Fix
    • Priority: Minor
    • Resolution: Fixed
    • Affects Version/s: 3.1.3
    • Fix Version/s: 3.1.6-RC1
    • Component/s: Events
    • Labels:
      None

      Description

      In the events core.posting_modify_submit_post_before and core.posting_modify_submit_post_after, the parameter 'error' does not have any impact in further execution, and particularly it does not achieve what is specified in the docblock: "Any error strings; a non-empty array aborts form submission". Similarly, the parameter 'submit' is useless here as well, as both events are within an "if $submit", therefore this parameter will always be true, and changing it will not have further impact.
      Please, remove both parameters from these events.

        Activity

        Hide
        nickvergessen Joas Schilling added a comment -

        Confirmed, we are inside if (!sizeof($error) && $submit)

        Either it should be removed, or fixed (in case of error)

        Show
        nickvergessen Joas Schilling added a comment - Confirmed, we are inside if (!sizeof($error) && $submit) Either it should be removed, or fixed (in case of error)
        Hide
        VSE Matt Friedman added a comment - - edited

        Then why are these event's docblocks saying: "This event allows you to define errors before/after the post action is performed"

        If that is their purpose, they should still receive the $error var even if it is currently empty. So an extension can just use it and not have to set a new one of their own.

        Show
        VSE Matt Friedman added a comment - - edited Then why are these event's docblocks saying: "This event allows you to define errors before/after the post action is performed" If that is their purpose, they should still receive the $error var even if it is currently empty. So an extension can just use it and not have to set a new one of their own.
        Hide
        javiexin javiexin added a comment -

        Problem is setting this $error var has absolutely no effect in following code, so it is empty when entering the event, and useless if filled in in the event.
        In my view, the docblocks are just copy&paste of other (previous) events.
        Either remove the parameter, or change code to make use of them.

        Show
        javiexin javiexin added a comment - Problem is setting this $error var has absolutely no effect in following code, so it is empty when entering the event, and useless if filled in in the event. In my view, the docblocks are just copy&paste of other (previous) events. Either remove the parameter, or change code to make use of them.
        Hide
        VSE Matt Friedman added a comment - - edited

        None of the vars in this event have any affect on the code following the event. And that is not the point of events. The point is to give an extension access to all important vars defined so far, so they can do something with them. An extension at this point could use this event to generate its own error handling, and never return to the core code, for example. It might theoretically even populate the $error var in the first event, that may be used by the second event.

        The point is to give extensions all the flexibility to do something - not just change a var and give it back to the core.

        Show
        VSE Matt Friedman added a comment - - edited None of the vars in this event have any affect on the code following the event. And that is not the point of events. The point is to give an extension access to all important vars defined so far, so they can do something with them. An extension at this point could use this event to generate its own error handling, and never return to the core code, for example. It might theoretically even populate the $error var in the first event, that may be used by the second event. The point is to give extensions all the flexibility to do something - not just change a var and give it back to the core.
        Hide
        javiexin javiexin added a comment -

        Compare the behaviour of this instance of the $error variable transferred to the events (nothing at all) with the one of the same variable in the event in L1240 of the same file, core.posting_modify_submission_errors.

        If you want to keep information from one event to the other, make it through a var in the listener class...

        If you need vars in the events is either to receive information from the core (in this case, nothing, as they are both empty) or to influence further execution of the core by modifying them (in this case, nothing again, as these variables are not used anywhere else in the execution path).

        Show
        javiexin javiexin added a comment - Compare the behaviour of this instance of the $error variable transferred to the events (nothing at all) with the one of the same variable in the event in L1240 of the same file, core.posting_modify_submission_errors. If you want to keep information from one event to the other, make it through a var in the listener class... If you need vars in the events is either to receive information from the core (in this case, nothing, as they are both empty) or to influence further execution of the core by modifying them (in this case, nothing again, as these variables are not used anywhere else in the execution path).
        Hide
        VSE Matt Friedman added a comment -

        As I already said, it really doesn't matter if it is empty (they are still defined) or never used again after the event (extensions can halt execution themselves by throwing their own error).

        That said, as I also already said, the point here is to decide what these events are for! Are they to define errors for error handling, or to modify something else during submit post...and make the correct corrections.

        Another example in theory, is an extension may tie the same function to two different events, and rely on the value of $submit to determine what to do. So even though $submit is guaranteed to be true here, doesn't mean we should therefore automatically remove it. Some extension might still have a need to know the value of $submit anyways.

        Show
        VSE Matt Friedman added a comment - As I already said, it really doesn't matter if it is empty (they are still defined) or never used again after the event (extensions can halt execution themselves by throwing their own error). That said, as I also already said, the point here is to decide what these events are for! Are they to define errors for error handling, or to modify something else during submit post...and make the correct corrections. Another example in theory, is an extension may tie the same function to two different events, and rely on the value of $submit to determine what to do. So even though $submit is guaranteed to be true here, doesn't mean we should therefore automatically remove it. Some extension might still have a need to know the value of $submit anyways.
        Hide
        javiexin javiexin added a comment - - edited

        At the very minimum, it should be made very clear in the docblock that these vars do NOT have any impact in code execution (specifically $error), as the current text indicates something very different.
        I did raise this issue when I was wrongly expecting this to happen, led to it by the event docblock text, and realized it was not the case by analyzing the core code. A lot of time wasted due to unclear comments.
        But, personally, I would certainly prefer the removal of these parameters.

        Show
        javiexin javiexin added a comment - - edited At the very minimum, it should be made very clear in the docblock that these vars do NOT have any impact in code execution (specifically $error), as the current text indicates something very different. I did raise this issue when I was wrongly expecting this to happen, led to it by the event docblock text, and realized it was not the case by analyzing the core code. A lot of time wasted due to unclear comments. But, personally, I would certainly prefer the removal of these parameters.

          People

          • Assignee:
            Marc Marc
            Reporter:
            javiexin javiexin
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development