MyBB Community Forums - newreply.php warning 1.8.36

In newreply.php, an error is generated.

<error>
	<dateline>1693899950</dateline>
	<datetime>2023-09-05 07:45:50 UTC -0500</datetime>
	<script>newreply.php(1530) : eval()'d code</script>
	<line>51</line>
	<type>2</type>
	<friendly_type>Warning</friendly_type>
	<message>Array to string conversion</message>
	<back_trace>#0  errorHandler->error() called at [/inc/class_error.php:153]
#1  errorHandler->error_callback() called at [/newreply.php(1530) : eval()'d code:51]
#2  eval() called at [/newreply.php:1530]
</back_trace>
</error>

eval'd code in template is line 50

<input type="hidden" name="quoted_ids" value="{$quoted_ids}" />

At different points in execution $quoted_ids is a string or an array. The qualification of is_array() and count() at line 862 is skipped if $quoted_ids becomes an empty array.

Laird, and thanks for your help! Wrote:862 tests for an array of one element or more. Thus, the implosion on 864 isn't reached for empty arrays. It should be, because otherwise empty arrays aren't converted to strings, causing the warning you were getting on line 50 of the newreply template, which references $quoted_ids (which at that point is sometimes an empty array instead of a string).

find, around line 873

			if(is_array($quoted_ids) && count($quoted_ids) > 0)
			{
				$quoted_ids = implode("|", $quoted_ids);
			}

and replace with

//			if(is_array($quoted_ids) && count($quoted_ids) > 0)
//			{
				$quoted_ids = implode("|", $quoted_ids);
//			}

essentially, comment out the conditional

I've fixed this in this PR: https://github.com/mybb/mybb/pull/4739

Reverting to standard 1.8.36 newreply.php then adding those lines gave me error-free handling of quick reply and edited post, as well as full editor creation and editing of another post. Thanks for the fix.

------

What's going on? 22,000 views of this thread? Are bots hitting it?

(2023-09-05, 07:03 PM)StefanT Wrote: [ -> ]I've fixed this in this PR: https://github.com/mybb/mybb/pull/4739

Here, you mean?

That seems to be in a different execution path (for the do_newreply action) to the one in which the bug reported by HLFadmin occurs (for the newreply or editdraft actions), and doesn't seem to do anything anyway ($quoted_ids never seems to be referenced again in the do_newreply action after the code you've inserted).

Am I missing something? If so, can you please explain what?

Thank you for pointing that out. On a closed look using count there makes no sense so I've just updated the PR to remove it in both instances. All those is_array calls in that part of the code are pretty useless.

Looks good to me. Agreed that those count()s and is_array()s needed to be removed, and I'm glad that you also picked up on that bogus MyBB::INPUT_INT, which HLFadmin and I had also flagged privately.

My only very pedantic and minor concern is that the implode() that you've added on what would be line 595 still seems redundant to core code given that $quoted_ids doesn't seem to be referenced afterwards in that execution path, but is potentially referenced by an existing plugin hooking in to newreply_do_newreply_end, which might be assuming it's still an array, and be broken by this addition.

Hey, I did warn that it was pedantic and minor!

$quoted_ids is defined as string in line 287 so there are already different data types. But it is a valid point.

(2023-09-06, 12:38 PM)StefanT Wrote: [ -> ]Thank you for pointing that out. On a closer look using count there makes no sense so I've just updated the PR to remove it in both instances. All those is_array calls in that part of the code are pretty useless.

Thank you for taking a closer look. Empirical evidence after implementing the fix yesterday shows a couple new replies slipped through last night with warnings.

I restored the core edit as in the original post.

In 24 hours of forum activity, I received no warnings, including when I tested by creating a thread with posts containing quotes from external threads as well as internal to the thread. I used every combination I could think of. Admittedly, I do not have the ability to test with a wide assortment of permissions. My forum is fairly basic where all usergroups have nearly the same posting privileges.