Bug #672

threadviews task won't disable from settings change

Added by Jesse Labrocca about 2 years ago. Updated almost 2 years ago.

Status:Closed Start date:01/10/2010
Priority:Normal Due date:
Assignee:Tom Moore % Done:

100%
Category:Admin Control Panel
Target version:1.4.12
Reproducibility:Always Database Type:
Reported In MyBB Version:1.4.12 Database Version:
PHP Version: SQA assignments:
Browser:

Description

If you turn on the setting for Delayed Thread Views the task for threadviews is enabled. However when you turn off the setting the task remains enabled.

I assume a logic problem in /admin/modules/config/settings.php

@ // If the delayedthreadviews setting was changed, enable or disable the tasks for it.
if($mybb->input['upsetting']['delayedthreadviews'] && $mybb->settings['delayedthreadviews'] != $mybb->input['upsetting']['delayedthreadviews']) {
if($mybb->input['upsetting']['delayedthreadviews'] == 0) {
$updated_task = array(
"enabled" => 0
);
}
else {
$updated_task = array(
"enabled" => 1
);
}
$db->update_query("tasks", $updated_task, "file='threadviews'");
}@

Tested on my blank beta site and 2 live sites.

Associated revisions

Revision 4720
Added by Tom Moore about 2 years ago

Fixes threadviews task won't disable from settings change (fixes:672)

Revision 4763
Added by Tom Moore about 2 years ago

Fixes threadviews task won't disable from settings change (fixes:672)

History

#1
Updated by Ryan Gordon about 2 years ago

This is intended. If you check the actual task file it checks the setting to make sure its on before it goes any further. This isn't a bug.

if($mybb->settings['delayedthreadviews'] != 1)
    {
        return;
    }

#2
Updated by Ryan Gordon about 2 years ago

Oh well, I guess you are kinda right. The logic problem is in if($mybb->input['upsetting']['delayedthreadviews']

It should instead check if it is isset I_ believe_

#3
Updated by Tom Moore about 2 years ago

  • Status changed from New to Assigned
  • Assignee set to Tom Moore
  • Target version set to 1.4.12

#4
Updated by Tom Moore about 2 years ago

  • Status changed from Assigned to Resolved
  • % Done changed from 0 to 100

Applied in changeset r4720.

#5
Updated by Huji Lee about 2 years ago

I think this is wrongly patched. See the code:

        if($mybb->input['upsetting']['delayedthreadviews'] && $mybb->settings['delayedthreadviews'] != $mybb->input['upsetting']['delayedthreadviews'])
        {
            if(isset($mybb->input['upsetting']['delayedthreadviews']))
            {
                $updated_task = array(
                    "enabled" => 0
                );
            }
            else
            {
                $updated_task = array(
                    "enabled" => 1
                );
            }
            $db->update_query("tasks", $updated_task, "file='threadviews'");
        }

If $mybb->input['upsetting']['delayedthreadviews'] is not set, the code won't get to the isset command, becuase the first if won't get executed at all.

I guess the correct fix is:

        if(isset($mybb->input['upsetting']['delayedthreadviews']) && $mybb->settings['delayedthreadviews'] != $mybb->input['upsetting']['delayedthreadviews'])
        {
            if($mybb->input['upsetting']['delayedthreadviews'] == 0)
            {
                $updated_task = array(
                    "enabled" => 0
                );
            }
            else
            {
                $updated_task = array(
                    "enabled" => 1
                );
            }
            $db->update_query("tasks", $updated_task, "file='threadviews'");
        }

#6
Updated by Ryan Gordon about 2 years ago

  • Status changed from Resolved to Feedback

#7
Updated by Tom Moore about 2 years ago

I tested the code before I commited it, Huji, but if it's wrong and it isn't fixed then feel free to change it.

#8
Updated by Ryan Gordon about 2 years ago

Tom Moore wrote:

I tested the code before I commited it, Huji, but if it's wrong and it isn't fixed then feel free to change it.

I think Huji is correct. Logically the original code block will always execute to "enabled" => 0 because if($mybb->input['upsetting']['delayedthreadviews']) will only produce true if is != 0. Then obviously the following nested isset($mybb->input['upsetting']['delayedthreadviews']) will return true. It should be checked to see if it is "isset" first (passed along with the form) then you actually challenge the setting with a question of "are you set to on or off?" and act based on that.

#9
Updated by Tom Moore about 2 years ago

  • Status changed from Feedback to Resolved

Applied in changeset r4763.

Done.

#10
Updated by Stefan T. almost 2 years ago

  • Status changed from Resolved to Closed

Also available in: Atom PDF