[Pushed] Editing announcement is broken when global variable "$announcement"

[Jones H]

If a plugin uses a global variable named "$announcement" the modcp edit announcement part will thrown the error "The specified announcement doesn't exist". That's because of this if. The reason for the if is to avoid that the announcement is loaded multiple times (it's already loaded in do_edit_announcement normally). However it fails when the variable is already in use. IMHO we should add a check for request_method == "post".

[Jones H]

Bump

[Destroy666]

Either the linked lines are outdated or I don't understand what's wrong.

EDIT: and if you mean the 2 conditionals above, which you most likely do - won't adding request_method == "post" to the 1st check skip the query when you're accessing the action directly, which would break it completely? Unless you meant adding that condition as an alternative rather than &&:

if(!isset($announcement) || $mybb->request_method == 'post')

but then this optimisation is kind of pointless since the query will be always executed.

EDIT2: the same thing happens probably in many places btw, I just remembered that I accidentally broke quick reply in one of my plugins by similarly globalizing $post['xxx'] in a global hook. This line fired off: https://github.com/mybb/mybb/blob/featur...ly.php#L86 Not sure if that's something we can/should fix everywhere.

[Jones H]

Yeah, meant the "isset" - if the request method is post, the variable/announcement is loaded already. If you're accessing it directly (without a post request) the query is always executed. So the if would be:

if(!isset($announcement) || $mybb->request_method != 'post')

Don't know about other places but IMHO we should fix those cases to - "announcement" is a very common variable name, especially for announcement managers (we have at least 3 of them on the mods site).

Edit: Probably even better to check for $mybb->input['action'] != 'do_edit_announcement'.

[Destroy666]

Edit: Probably even better to check for $mybb->input['action'] != 'do_edit_announcement'.

That would surely not work because the action is edit_announcement at that point.

if(!isset($announcement) || $mybb->request_method != 'post') 

seems ok though.

[Jones H]

Ah, yeah, forgot that we change that always...

[Jones H]

Hi,

Thank you for your report. We have pushed this issue to our Github repository for further analysis where you can track our commits and progress with fixing this bug. Discussions regarding this bug may also take place there too.

Follow this link to visit the issue on Github: https://github.com/mybb/mybb/issues/2129

Thanks for contributing to MyBB!

Regards,
The MyBB Group