Skip to content

Commit

Permalink
Fixes several bugs with attachments at once (mybb#3926)
Browse files Browse the repository at this point in the history 
* This commit fixes several bugs with attachments at once:

1. When the "Post Thread" or "Post Reply" button was clicked after an
   attachment had been selected with the "Choose Files" button, the
   attachment was not saved.

2. When "Add Attachment" was clicked after a previous attachment had
   been removed via AJAX and a new attachment had been selected with
   the "Choose Files" button, the new attachment was not saved. I
   reported this issue here:

   https://community.mybb.com/thread-227178.html

3. When Javascript was disabled and "Remove", "Unapprove", or "Approve"
   was clicked, the corresponding action was not performed. This bug
   was reported in issue mybb#3675.

These problems were caused by:

1. Various typos. Correcting the typos resolves the problem:

1.1. A typo - already noted in issue mybb#3886 - in the conditionals in
     editpost.php, newreply.php, and newthread.php (singular should
     be plural):

     `$_FILES['attachment']` should be `$_FILES['attachments']`

1.2. A typo in two conditionals in editpost.php (as noted in issue
     mybb#3675):

     `$mybb->input['submit']` should be `$mybb->input['submitbutton']`

2. The setting in jscripts/post.js of the `attachmentaid` input when
   removing an attachment, which, when the new attachment was
   submitted, caused those same conditionals as in 1.1 to fail due to
   the test:

   `!$mybb->get_input('attachmentaid', MyBB::INPUT_INT)`

   Removing that test resolves the problem without any side-effects.
   An alternative was to unset `attachmentaid` in post.js after
   the removal operation completed, however, it seems better to
   delete code than to add it.

3. The lack of a value for the `attachmentaid` and `attachmentact`
   inputs. This is fixed in this commit by:

   3.1. Adjusting the `name` property of the remove, approve, and
        unapprove submit buttons such that the attachment ID is
	appended after an underscore.

   3.2. Adding a function `detect_attachmentact()` which detects
        when an input with such a name is present, extracts the
	attachment ID from it, and then sets the missing values
	for `attachmentaid` and `attachmentact`.

* Apply suggestions from code review

Co-Authored-By: Euan <[email protected]>

* Unset attachment[aid/act] in post.js after removing an attachment.

In the first commit in this PR, I wrote:

  An alternative was to unset `attachmentaid` in post.js after
  the removal operation completed, however, it seems better to
  delete code than to add it.

In hindsight, it seems best to make this change on top of the
previous one, and to in addition unset `attachmentact`, to
prevent incorrect signalling to server-side code.

* Enhances "Remove" attachment support.

* Provides support for existing themes which have not yet changed
  their templates to mirror the changes we've just made to the
  'post_attachments_attachment_remove' template.
* Provides support for fading out the removed attachment with
  Javascript when it is unapproved.

* Simplified the support in post.js for 3rd-party themes.

* Updated template 'post_attachments_attachment_unapproved'.

It had hard-coded HTML for the "Remove" button, such
that the recent changes we've made to that button's
"name" attribute in the
'post_attachments_attachment_remove' template weren't
being reflected.

* Also regenerate header in editpost.php when removing an attachment.

As caught by @yuliu here:

mybb#3926 (comment)

* Save attachments on "Preview Post" and "Save as Draft".

* Replace mod queue in $header rather than regen $header.

In editpost.php, when an attachment is removed or (un)approved and
Javascript is not enabled.

* Removes header regen from editpost.php.

Instead, blanks the header's moderation queue in global.php when the
current page is editpost.php.

* Removes a redundant line in global.php.

Co-authored-by: Euan <[email protected]>
  • Loading branch information
@lairdshaw @euantorano
lairdshaw and euantorano authored Jun 14, 2020
1 parent 465419b commit 80cba4a
Show file tree
Hide file tree
Showing 7 changed files with 63 additions and 20 deletions.
8 changes: 5 additions & 3 deletions editpost.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@
}

$attacherror = '';
if($mybb->settings['enableattachments'] == 1 && !$mybb->get_input('attachmentaid', MyBB::INPUT_INT) && ($mybb->get_input('newattachment') || $mybb->get_input('updateattachment') || ($mybb->input['action'] == "do_editpost" && isset($mybb->input['submit']) && $_FILES['attachment'])))
if($mybb->settings['enableattachments'] == 1 && ($mybb->get_input('newattachment') || $mybb->get_input('updateattachment') || ((($mybb->input['action'] == "do_editpost" && isset($mybb->input['submitbutton'])) || ($mybb->input['action'] == "editpost" && isset($mybb->input['previewpost']))) && $_FILES['attachments'])))
{
// Verify incoming POST request
verify_post_check($mybb->get_input('my_post_key'));
Expand Down Expand Up @@ -220,12 +220,14 @@
}

// If we were dealing with an attachment but didn't click 'Update Post', force the post edit page again.
if(!isset($mybb->input['submit']))
if(!isset($mybb->input['submitbutton']))
{
$mybb->input['action'] = "editpost";
}
}

detect_attachmentact();

if($mybb->settings['enableattachments'] == 1 && $mybb->get_input('attachmentaid', MyBB::INPUT_INT) && isset($mybb->input['attachmentact']) && $mybb->input['action'] == "do_editpost" && $mybb->request_method == "post") // Lets remove/approve/unapprove the attachment
{
// Verify incoming POST request
Expand Down Expand Up @@ -256,7 +258,7 @@
exit();
}

if(!isset($mybb->input['submit']))
if(!isset($mybb->input['submitbutton']))
{
$mybb->input['action'] = "editpost";
}
Expand Down
7 changes: 5 additions & 2 deletions global.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -745,8 +745,11 @@
}
}

// Get awaiting moderation queue stats
if($can_access_moderationqueue || ($mybb->user['ismoderator'] && $mybb->usergroup['canmodcp'] == 1 && $mybb->usergroup['canmanagemodqueue'] == 1))
// Get awaiting moderation queue stats, except if the page is editpost.php,
// because that page can make changes - (un)approving attachments, or deleting
// unapproved attachments - that would invalidate anything generated here.
// Just leave this queue notification blank for editpost.php.
if(!(defined('THIS_SCRIPT') && THIS_SCRIPT == 'editpost.php') && ($can_access_moderationqueue || ($mybb->user['ismoderator'] && $mybb->usergroup['canmodcp'] == 1 && $mybb->usergroup['canmanagemodqueue'] == 1)))
{
$unapproved_posts = $unapproved_threads = 0;
$query = $db->simple_select("posts", "replyto", "visible = 0");
Expand Down
32 changes: 32 additions & 0 deletions inc/functions_post.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1067,3 +1067,35 @@ function return_bytes($val) {

return intval($val);
}

/**
* Detects whether an attachment removal/approval/unapproval
* submit button was pressed (without triggering an AJAX request)
* and sets inputs accordingly (as for an AJAX request).
*/
function detect_attachmentact()
{
global $mybb;

foreach($mybb->input as $key => $val)
{
if(strpos($key, 'rem_') === 0)
{
$mybb->input['attachmentaid'] = (int)substr($key, 4);
$mybb->input['attachmentact'] = 'remove';
break;
}
elseif(strpos($key, 'approveattach_') === 0)
{
$mybb->input['attachmentaid'] = (int)substr($key, 14);
$mybb->input['attachmentact'] = 'approve';
break;
}
elseif(strpos($key, 'unapproveattach_') === 0)
{
$mybb->input['attachmentaid'] = (int)substr($key, 16);
$mybb->input['attachmentact'] = 'unapprove';
break;
}
}
}
14 changes: 7 additions & 7 deletions install/resources/mybb_theme.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9448,14 +9448,14 @@ if(use_xmlhttprequest == "1")
<td class="trow2" width="60%" style="white-space: nowrap">{$attachment['filename']} ({$attachment['size']})</td>
<td class="trow2" style="white-space: nowrap; text-align: center;">{$attach_mod_options} {$attach_rem_options} {$postinsert}</td>
</tr>]]></template>
<template name="post_attachments_attachment_mod_approve" version="120"><![CDATA[<input type="submit" class="button" name="approveattach" value="{$lang->approve_attachment}" onclick="return Post.attachmentAction({$attachment['aid']},'approve');" />]]></template>
<template name="post_attachments_attachment_mod_unapprove" version="120"><![CDATA[<input type="submit" class="button" name="unapproveattach" value="{$lang->unapprove_attachment}" onclick="return Post.attachmentAction({$attachment['aid']},'unapprove');" />]]></template>
<template name="post_attachments_attachment_mod_approve" version="1823"><![CDATA[<input type="submit" class="button" name="approveattach_{$attachment['aid']}" value="{$lang->approve_attachment}" onclick="return Post.attachmentAction({$attachment['aid']},'approve');" />]]></template>
<template name="post_attachments_attachment_mod_unapprove" version="1823"><![CDATA[<input type="submit" class="button" name="unapproveattach_{$attachment['aid']}" value="{$lang->unapprove_attachment}" onclick="return Post.attachmentAction({$attachment['aid']},'unapprove');" />]]></template>
<template name="post_attachments_attachment_postinsert" version="1809"><![CDATA[<input type="button" class="button" name="insert" value="{$lang->insert_attachment_post}" onclick="$('#message').sceditor('instance').insertText('[attachment={$attachment['aid']}]'); return false;" />]]></template>
<template name="post_attachments_attachment_remove" version="1604"><![CDATA[<input type="submit" class="button" name="rem" value="{$lang->remove_attachment}" onclick="return Post.removeAttachment({$attachment['aid']});" />]]></template>
<template name="post_attachments_attachment_unapproved" version="120"><![CDATA[<tr>
<template name="post_attachments_attachment_remove" version="1823"><![CDATA[<input type="submit" class="button" name="rem_{$attachment['aid']}" value="{$lang->remove_attachment}" onclick="return Post.removeAttachment({$attachment['aid']});" />]]></template>
<template name="post_attachments_attachment_unapproved" version="1823"><![CDATA[<tr id="attachment_{$attachment['aid']}">
<td class="trow_shaded" width="1" align="center">{$attachment['icon']}</td>
<td class="trow_shaded" style="white-space: nowrap"><em><a href="attachment.php?aid={$attachment['aid']}" target="_blank">{$attachment['filename']}</a> ({$attachment['size']})</em></td>
<td class="trow_shaded" style="white-space: nowrap; text-align: center;">{$attach_mod_options} <input type="submit" class="button" name="rem" value="{$lang->remove_attachment}" onclick="return Post.removeAttachment({$attachment['aid']});" /> {$postinsert}</td>
<td class="trow_shaded" style="white-space: nowrap; text-align: center;">{$attach_mod_options} {$attach_rem_options} {$postinsert}</td>
</tr>]]></template>
<template name="post_attachments_new" version="1818"><![CDATA[<tr>
<td class="trow1" width="1"><img src="{$theme['imgdir']}/paperclip.png" alt="" /></td>
Expand Down Expand Up @@ -9596,14 +9596,14 @@ if(use_xmlhttprequest == "1")
</table>
</td>
</tr>]]></template>
<template name="post_javascript" version="1822"><![CDATA[
<template name="post_javascript" version="1823"><![CDATA[
<script type="text/javascript">
lang.attachment_too_many_files = "{$lang->attachment_too_many_files}";
lang.attachment_too_big_upload = "{$lang->attachment_too_big_upload}";
php_max_upload_size = {$php_max_upload_size};
php_max_file_uploads = {$php_max_file_uploads};
</script>
<script type="text/javascript" src="{$mybb->asset_url}/jscripts/post.js?ver=1822"></script>
<script type="text/javascript" src="{$mybb->asset_url}/jscripts/post.js?ver=1823"></script>
]]></template>
<template name="post_prefixselect_multiple" version="1800"><![CDATA[<select name="threadprefix[]" multiple="multiple" size="5">
<option value="any"{$any_selected}>{$lang->any_prefix}</option>
Expand Down
4 changes: 3 additions & 1 deletion jscripts/post.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ var Post = {
document.input.attachmentaid.value = aid;
document.input.attachmentact.value = "remove";

var form = $('input[name=rem]').parents('form');
var form = $('input[name^=\'rem\']').parents('form');

if(use_xmlhttprequest != 1)
{
Expand Down Expand Up @@ -146,6 +146,8 @@ var Post = {
$(this).remove();
});
}
document.input.attachmentaid.value = '';
document.input.attachmentact.value = '';
}
});
}
Expand Down
10 changes: 6 additions & 4 deletions newreply.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@

$errors = array();
$maximageserror = $attacherror = '';
if($mybb->settings['enableattachments'] == 1 && !$mybb->get_input('attachmentaid', MyBB::INPUT_INT) && ($mybb->get_input('newattachment') || $mybb->get_input('updateattachment') || ($mybb->input['action'] == "do_newreply" && $mybb->get_input('submit') && $_FILES['attachment'])))
if($mybb->settings['enableattachments'] == 1 && ($mybb->get_input('newattachment') || $mybb->get_input('updateattachment') || ((($mybb->input['action'] == "do_newreply" && $mybb->get_input('submit')) || ($mybb->input['action'] == "newreply" && isset($mybb->input['previewpost'])) || isset($mybb->input['savedraft'])) && $_FILES['attachments'])))
{
// Verify incoming POST request
verify_post_check($mybb->get_input('my_post_key'));
Expand All @@ -226,14 +226,16 @@
$errors = $ret['errors'];
}

// If we were dealing with an attachment but didn't click 'Post Reply', force the new reply page again.
if(!$mybb->get_input('submit'))
// If we were dealing with an attachment but didn't click 'Post Reply' or 'Save as Draft', force the new reply page again.
if(!$mybb->get_input('submit') && !$mybb->get_input('savedraft'))
{
eval("\$editdraftpid = \"".$templates->get("newreply_draftinput")."\";");
$mybb->input['action'] = "newreply";
}
}

detect_attachmentact();

// Remove an attachment.
if($mybb->settings['enableattachments'] == 1 && $mybb->get_input('attachmentaid', MyBB::INPUT_INT) && $mybb->get_input('attachmentact') == "remove")
{
Expand Down Expand Up @@ -700,7 +702,7 @@

$quote_ids = $multiquote_external = '';
// If this isn't a preview and we're not editing a draft, then handle quoted posts
if(empty($mybb->input['previewpost']) && !$reply_errors && $mybb->input['action'] != "editdraft" && !$mybb->get_input('attachmentaid', MyBB::INPUT_INT) && !$mybb->get_input('newattachment') && !$mybb->get_input('updateattachment') && !$mybb->get_input('rem'))
if(empty($mybb->input['previewpost']) && !$reply_errors && $mybb->input['action'] != "editdraft" && !$mybb->get_input('attachmentaid', MyBB::INPUT_INT) && !$mybb->get_input('newattachment') && !$mybb->get_input('updateattachment'))
{
$message = '';
$quoted_posts = array();
Expand Down
8 changes: 5 additions & 3 deletions newthread.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@
$maximageserror = $attacherror = '';

// Handle attachments if we've got any.
if($mybb->settings['enableattachments'] == 1 && !$mybb->get_input('attachmentaid', MyBB::INPUT_INT) && ($mybb->get_input('newattachment') || $mybb->get_input('updateattachment') || ($mybb->input['action'] == "do_newthread" && $mybb->get_input('submit') && $_FILES['attachment'])))
if($mybb->settings['enableattachments'] == 1 && ($mybb->get_input('newattachment') || $mybb->get_input('updateattachment') || ((($mybb->input['action'] == "do_newthread" && $mybb->get_input('submit')) || ($mybb->input['action'] == "newthread" && isset($mybb->input['previewpost'])) || isset($mybb->input['savedraft'])) && $_FILES['attachments'])))
{
// Verify incoming POST request
verify_post_check($mybb->get_input('my_post_key'));
Expand All @@ -180,13 +180,15 @@
$errors = $ret['errors'];
}

// If we were dealing with an attachment but didn't click 'Post Thread', force the new thread page again.
if(!$mybb->get_input('submit'))
// If we were dealing with an attachment but didn't click 'Post Thread' or 'Save as Draft', force the new thread page again.
if(!$mybb->get_input('submit') && !$mybb->get_input('savedraft'))
{
$mybb->input['action'] = "newthread";
}
}

detect_attachmentact();

// Are we removing an attachment from the thread?
if($mybb->settings['enableattachments'] == 1 && $mybb->get_input('attachmentaid', MyBB::INPUT_INT) && $mybb->get_input('attachmentact') == "remove")
{
Expand Down

0 comments on commit 80cba4a

Please sign in to comment.