Please help me to understand and change this small piece of code

[imthebest]

Hi guys,

I have the following code (just pasting the relevant parts)

$days = XenForo_Application::get('options')->autoLockDays;

$timestamp = time() - (86400 * $days);

            $db = XenForo_Application::get('db');  
      
            $threads = $db->fetchAll("
            SELECT xf_thread.thread_id
            FROM xf_thread
            INNER JOIN xf_node ON xf_node.node_id = xf_thread.node_id
            WHERE xf_thread.last_post_date < " . $timestamp . "
            AND xf_thread.sticky = 0
            $whereclause
            ");              

            foreach ($threads AS $k => $v)
            {
                $db->query('
                    UPDATE xf_thread SET
                        discussion_open = "0"
                    WHERE thread_id = ?
                ', $v);
            }

That code allows me to set the value for the $days variable at the ACP. Then here is the explanation from the coder:

The Days value determine when the thread will be locked. For example if the thread never gets any replies, it will be locked X days after the thread creation date. Otherwise the last reply date plus the Days determine when the thread will be locked.

I want to change the behavior so threads will be locked after X days of their creation date without having in account the last reply date at all. I think I need to change:

WHERE xf_thread.last_post_date < " . $timestamp . "

To:

WHERE xf_thread.post_date < " . $timestamp . "

However I'm not sure what the second line of this code snippet does (the one that defines the $timestamp variable) and how it relates to the WHERE clause of the SQL query. My understanding is that the code is setting $timestamp to the current time minus the amount of days specified at the ACP (something like "X days ago"). But now on the query what are you evaluating? If the xf_thread.last_post_date is less than "X days ago"? It shouldn't be the contrary? I mean > instead of <?

Sorry I suck in math, I hate math.

Source: Auto Lock 1.1

Thanks.

 

[whynot]

There is a thread for this addon: https://xenforo.com/community/threads/auto-lock.79208/

 

[imthebest]

Yes, I know about the thread for the resource however I want to understand the code in order to make a minimal change to it. Thanks anyway.

 

[Ark Royal]

Whenever the code is run timestamp will be set to $days earlier.
So lets assume you run it now and $days is 5 than timestamp would be the current time on 19th September.

The WHERE statement is now comparing the time of the last post against 19th September and if it is before, ie <, then the thread is locked as it has been at least 5 days since the last post.

 

[Snog]

Maybe this will help...

time() is an integer number for the current date. It looks something like this...
1443095563

The 86400 is the number of seconds in a day.

So 86400 times 5 = 432000

Subtract 432000 from time() and you get the date 5 days earlier.

 

[imthebest]

Thanks guys. Another question: if the code goal is to close those threads that are older than X days, would it make sense to add:

AND discussion_open = 1

At the end of this?

            SELECT xf_thread.thread_id
            FROM xf_thread
            INNER JOIN xf_node ON xf_node.node_id = xf_thread.node_id
            WHERE xf_thread.last_post_date < " . $timestamp . "
            AND xf_thread.sticky = 0

My understanding is that this will limit the amount of items to work with thus reducing server load... but I'm also worried about the fact that adding more conditionals to the WHERE clause could cause a performance impact because MySQL needs to be more accurate and also I'm not sure if discussion_open is "indexed"?

At the end, the code will do the following:

            foreach ($threads AS $k => $v)
            {
                $db->query('
                    UPDATE xf_thread SET
                        discussion_open = "0"
                    WHERE thread_id = ?
                ', $v);
            }

So if I pass the full thread list then it will change the discussion_open value to "0" for those threads that currently have the value "1" (my understanding is that on the second step MySQL will have to go row by row and determine whether it needs to apply the change or not). However if on the first step I add the "AND discussion_open = 1" then I'll be passing a curated list of threads to the second step and it will not need to "analyze" more and just apply the change.

What is going to be better for performance? Or in the practice it is going to be the same?

 

[AndyB]

Or in the practice it is going to be the same?

This.

In practice the speed difference will be .00001 different, so it's not going to matter much.

 

[Xon]

You are better off writing it as a single statement rather than a select followed by a large number of updates.

Something like:

$db->query("
  UPDATE  xf_thread
  SET discussion_open = 0
  WHERE discussion_open = 1
      AND xf_thread.last_post_date < ?
      AND xf_thread.sticky = 0
     $whereclause
  ", $timestamp);

(No need to join to xf_node)

Make sure you test rather than blindly accept the code, this was written as a proof-of-concept and not tested!

 

[PaulB]

It's worth noting that XenForo uses Unix timestamps. A Unix timestamp stores a point in time as the number of seconds since January 1, 1970 00:00 GMT. (UTC didn't come into existence until 1972.)

Let's break down the confusing line:

$timestamp = time() - (86400 * $days);

In the order the tokens are evaluated in:

1. time() obtains the current time, as a Unix timestamp. However, you should be using XenForo_Application::$time instead. The difference is usually negligible, but XenForo_Application::$time is set at the start of the request, which can help avoid race conditions in some cases. It probably won't matter here.
2. 86400 is the number of seconds in a Gregorian day. (Our modern calendar system is mostly based on the Gregorian system, with a few adjustments for leap seconds that aren't relevant here.)
3. $days is the number of days we want to convert to seconds.
4. 86400 and $days are multiplied to get the number of seconds in "$days" days. The parentheses around this expression are unnecessary; they don't change the order of anything because multiplication is performed before subtraction.
5. We take that total and subtract it from the result of time(), then store it in $timestamp.
6. $timestamp now holds our cutoff point as a Unix timestamp. Any timestamp prior to this--a lower number--will be included in our selection. These selected threads will then be updated.

@Xon is correct; you are much better off combining all of this into a single query. Running many queries in a loop is a very bad idea. There's a lot of unnecessary overhead, and it's a great way to crash your site. These design flaws are particularly noticeable on high-traffic sites.

You can do almost everything here in MySQL:

$db->query("
	UPDATE xf_thread
	SET discussion_open = 0
	WHERE
		post_date < UNIX_TIMESTAMP() - 86400 * ?
		AND sticky = 0
		AND discussion_open = 1
", XenForo_Application::get('options')->autoLockDays);

Note how there's no loop in PHP.

Depending on the indexes, it may actually be ideal to omit the "AND discussion_open = 1" line. You'd need to test this with your configuration, particularly if you plan to add an index to speed up this query. I'd recommend adding a composite index on (post_date, sticky, discussion_open) when your add-on is installed:

$db->query("
	ALTER TABLE xf_thread
	ADD INDEX post_date_sticky_discussion_open (post_date, sticky, discussion_open)
");

 

[AndyB]

You can do almost everything here in MySQL:

$db->query("
    UPDATE xf_thread
    SET discussion_open = 0
    WHERE
        post_date < ?
        AND sticky = 0
        AND discussion_open = 1
", XenForo_Application::get('options')->autoLockDays);

Hi Xenexer,

Thank you for sharing your knowledge.

The above code I think would not work, the following line:

XenForo_Application::get('options')->autoLockDays);

is going to return a number like 365 for the number of days, we need a Unix Timestamp in this part of the query.

 

[PaulB]

Hi Xenexer,

Thank you for sharing your knowledge.

The above code I think would not work, the following line:

XenForo_Application::get('options')->autoLockDays);

is going to return a number like 365 for the number of days, we need a Unix Timestamp in this part of the query.

Sorry, I left out part of the query. I'll edit it now.

 

[AndyB]

I'll edit it now.

Thank you.

That sure is clean code.