1. This site uses cookies. By continuing to use this site, you are agreeing to our use of cookies. Learn More.

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

Discussion in 'XenForo Development Discussions' started by imthebest, Sep 24, 2015.

  1. imthebest

    imthebest Formerly Super120

    Hi guys,

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

    Code:
    $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:

    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.
     
    Last edited: Sep 24, 2015
  2. whynot

    whynot Well-Known Member

  3. imthebest

    imthebest Formerly Super120

    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.
     
  4. Ark Royal

    Ark Royal Active Member

    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.
     
    imthebest likes this.
  5. Snog

    Snog Well-Known Member

    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 and Steve F like this.
  6. imthebest

    imthebest Formerly Super120

    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:

    Code:
    AND discussion_open = 1
    At the end of this?

    Code:
                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:

    Code:
                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?
     
    Last edited: Sep 24, 2015
  7. AndyB

    AndyB Well-Known Member

    This.

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

    Xon Well-Known Member

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

    Something like:
    Code:
    $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!
     
    Robust, Cyb3r, Snog and 2 others like this.
  9. Zenexer

    Zenexer Active Member

    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:
    PHP:
    $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:
    PHP:
    $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:
    PHP:
    $db->query("
        ALTER TABLE xf_thread
        ADD INDEX post_date_sticky_discussion_open (post_date, sticky, discussion_open)
    "
    );
     
    Last edited: Sep 26, 2015
    imthebest likes this.
  10. AndyB

    AndyB Well-Known Member

    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.
     
  11. Zenexer

    Zenexer Active Member

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

    AndyB Well-Known Member

    Thank you.

    That sure is clean code.
     

Share This Page