Database access - is this right?

[Krupski Roger]

Hi all,

I have a text size selector on my board that saves the user's selection in the DB. In the database is a tiny unsigned INT with a default of '12" in the "phpbb_users" section:

Code: Select All

`user_view_text_size` tinyint(1) unsigned NOT NULL DEFAULT '12',

So now, please take a look at this code (the DB writing part - highlighted in red) and please tell me if I did it correctly. It works, but is it right and did I leave out any steps?

Code: Select All

function text_size()
    {
        global $db, $user, $template;

        $name = 'pxsize'; // select name

        $fs = request_var($name, 0, false, false); // check for a user selection

        if ($fs) // if user selection, write it to the db
        {
            $sql_ary = array( 'user_view_text_size' => $fs );
            $sql = 'UPDATE ' . USERS_TABLE . '
            SET ' . $db->sql_build_array('UPDATE', $sql_ary) . '
            WHERE user_id = ' . $user->data['user_id'];
            $db->sql_query($sql);
        }
        else // no user selection, use current value
        {
            $fs = $user->data['user_view_text_size'];
        }

        $s_view_text_size = '<select name="' . $name . '">'; // generate the select dropdown

        for ($i = 8; $i <= 24; $i++)
        {
            $s_view_text_size .= '<option value="' . $i . '"';
            $s_view_text_size .= ($i == $fs) ? ' selected="selected"' : '';
            $s_view_text_size .= '>' . $i . '</option>';
        }

        $s_view_text_size .= '</select>';

        $template->assign_vars(array( 'S_VIEW_TEXT_SIZE' => $s_view_text_size )); // return it to the template

        return $fs; // return size value to set the board body fontsize
    }

Thanks!

-- Roger

[Handyman]

Yes, that is correct.
I would add an int cast to the $user->data['user_id']. Even though that should be an int, I never trust anything going into the database and always either clean it (which sql_build_array does) or cast it (if it's an integer and not sent through the build array.

Code: Select all


$sql_ary = array(
    'user_view_text_size'    => $fs,
);

$sql = 'UPDATE ' . USERS_TABLE . '
    SET ' . $db->sql_build_array('UPDATE', $sql_ary) . '
    WHERE user_id = ' . (int) $user->data['user_id'];
 

[Krupski Roger]

Yes, that is correct.
I would add an int cast to the $user->data['user_id']. Even though that should be an int, I never trust anything going into the database and always either clean it (which sql_build_array does) or cast it (if it's an integer and not sent through the build array.

I see. I'll do that. Thanks!

-- Roger

[Krupski Roger]

Yes, that is correct.

Let me ask you one more thing:

I see code like this:

Code: Select all

    $result = $db->sql_query($sql);
    ....
    ....
    ....
    $db->sql_freeresult($result);


While other times it's just:

Code: Select all

    $db->sql_query($sql);


What is the purpose of "freeresult($result)"?

Thanks.

-- Roger

[topdown]

$db->sql_freeresult() should always be run after you are done with the query results.

It frees the memory that was holding the query results.
Its the same as PHP's http://php.net/manual/en/function.mysql-free-result.php

[Krupski Roger]

$db->sql_freeresult() should always be run after you are done with the query results.

It frees the memory that was holding the query results.
Its the same as PHP's http://php.net/manual/en/function.mysql-free-result.php

So then my code *should* be like this:

Code: Select all


            $sql_ary = array( 'user_view_text_size' => $fs );
            $sql = 'UPDATE ' . USERS_TABLE . '
            SET ' . $db->sql_build_array('UPDATE', $sql_ary) . '
            WHERE user_id = ' . (INT) $user->data['user_id'];
            $result = $db->sql_query($sql);
            $db->sql_freeresult($result);
 

Correct?

[Handyman]

You don't need the freeresult for an update statement. It's for select statements to free the memory from query results.

[Krupski Roger]

You don't need the freeresult for an update statement. It's for select statements to free the memory from query results.

OK I get it now. There is nothing TO free with an update statement.

Thank you.