How can I optimize my PHP code for better performance

PHP
1

I am very new to PHP and have tried various techniques to optimize this code when clicking on the export button to download a csv report. I’m not sure why the previous developer did it this way.

Is there a better why in PHP to make this code better? Willing to understand and learn from an PHP expert.

The database is MYSQL.

$coursefilterid = $_GET['course'];

            $conn = new mysqli($host, $username, $password, $database);
            if ($conn->connect_error) {
                die("Connection failed: " . $conn->connect_error);
            }

            $sqluserenrolled = "select mdl_user.username, mdl_user_enrolments.userid as enrolleduserid, mdl_enrol.courseid from mdl_user_enrolments
    Inner Join
    mdl_enrol
    on 
    mdl_enrol.id = mdl_user_enrolments.enrolid
    Inner Join
    mdl_user
    on
    mdl_user.id = mdl_user_enrolments.userid
    where  
    mdl_enrol.courseid = '" . $coursefilterid . "'
    order by mdl_user.username
    
    ";

            $queryenrolleduser = mysqli_query($conn, $sqluserenrolled);

            ?>

<html>
<head>
</head>
<body>

    <form method="post"
        action="<?php echo "userlistssiexport.php?id=$coursefilterid"?>">
        <input type="hidden" name="exportcourseid"
            value="<?php echo $coursefilterid;?>"> <input type="hidden"
            name="sessid" value="<?php echo $USER->sesskey;?>"> <input
            class="btn btn-primary" type="submit" name="submit"
            value="<?php echo "Export";?>">
    </form>


<?php
            $noteid = "";
            $cmId = "";
            ?>
          
    <table class="data-table">
        <caption class="title">User info</caption>
        <thead>
            <tr>
                <th>Username</th>
                <th>Firstname</th>
                <th>Lastname</th>
                <th>Email</th>
                <th>Last login</th>
                <th>Createddate</th>
                <th>Position</th>
                <th>Organization</th>
                <th>Certificate Request Date</th>
                <th>Role1</th>
                <th>Role2</th>
                <th>Role3</th>
            </tr>
        </thead>
        <tbody>

    <?php

            while ($row = mysqli_fetch_array($queryenrolleduser)) {

                $enrolleduserid = $row['enrolleduserid'];

                $sql = "select mdl_user.username as username, 
        mdl_user.firstname as firstname,
        mdl_user.lastname as lastname,
        mdl_user.email as email,
       mdl_user.lastlogin as lastaccess,
        mdl_user.timecreated as createddate,
        mdl_user_info_data.data as position 
        
        
        from mdl_user
        Inner Join 
        mdl_user_info_data
        on
        mdl_user_info_data.userid = mdl_user.id
        Inner Join
        mdl_user_info_field
        on 
        mdl_user_info_field.id = mdl_user_info_data.fieldid
        Inner Join
        mdl_user_lastaccess
        on
        mdl_user_lastaccess.userid = mdl_user.id
        
        
        where  mdl_user_info_field.id = 1
        and
        mdl_user.deleted = 0
        and
        mdl_user.id = '" . $enrolleduserid . "'
        group by mdl_user.username
        order by mdl_user.username
    
        ";
                $query = mysqli_query($conn, $sql);

                if (! $query) {
                    die('SQL Error: ' . mysqli_error($conn));
                } else {}

                ?>

        <?php
                $no = 1;
                $total = 0;
                $username = '';
                $coursename = '';
                $content = '';
                $modulename = '';
                $organization = '';
                $userid = '';
                $certificatedate = '';
                $userrole = '';
                $enrolleduserid = '';

                while ($row = mysqli_fetch_array($query)) {
                    // Do something here
                    $username = $row['username'];
                    $coursename = $row['coursename'];
                    $content = $row['content'];
                    $noteid = $row['noteid'];
                    // $notedatetime = date("d/m/y g:i (A)", $row['notedate']);
                    $notedatetime = date("D M j Y G:i A", $row['notedate']);
                    $lastaccess = date("D M j Y G:i A", $row['lastaccess']);
                    $createddate = date("D M j Y G:i A", $row['createddate']);
                    $datafile = $username . $coursename . $content;

                    echo '<tr>      
                    <td>' . $row['username'] . '</td>
                    <td>' . $row['firstname'] . '</td>
                    <td>' . $row['lastname'] . '</td>
                    <td>' . $row['email'] . '</td>                          
                    <td>' . $lastaccess . '</td>                                    
                    <td>' . $createddate . '</td>   
                    <td>' . $row['position'] . '</td>    
                                                    
                ';
                    $modid = $row['contextid'];
                    // Get module name

                    $sqlmodule = "select mdl_user.username as username, 
        mdl_user.firstname as firstname,
        mdl_user.lastname as lastname,
        mdl_user.email as email,
        FROM_UNIXTIME(mdl_user_lastaccess.timeaccess) as lastaccess,
        FROM_UNIXTIME(mdl_user.timecreated) as createddate,
        mdl_user_info_data.data as organization 
        
        
        from mdl_user
        Inner Join 
        mdl_user_info_data
        on
        mdl_user_info_data.userid = mdl_user.id
        Inner Join
        mdl_user_info_field
        on 
        mdl_user_info_field.id = mdl_user_info_data.fieldid
        Inner Join
        mdl_user_lastaccess
        on
        mdl_user_lastaccess.userid = mdl_user.id
        
        
        where  mdl_user_info_field.id = 3
        and
        mdl_user.deleted = 0
        and
        mdl_user.username ='" . $username . "'";

                    $querymodule = mysqli_query($conn, $sqlmodule);
                    ?>              
            <?php
                    $modulenamelink = "";
                    while ($row = mysqli_fetch_array($querymodule)) {

                        $organization = $row['organization'];
                    }

                    echo '<td>' . $organization . '</td>';

                    $sqlCertificateDateuid = "select id from mdl_user where username = '" . $username . "'";

                    $queryCertificateDateuid = mysqli_query($conn, $sqlCertificateDateuid);
                    while ($row = mysqli_fetch_array($queryCertificateDateuid)) {

                        $userid = $row['id'];
                    }

                    $sqlcertificatedate = "select * from mdl_certificateemail where userid = '" . $userid . "'
        and courseid = '" . $coursefilterid . "'";

                    $querycertificaterequestdate = mysqli_query($conn, $sqlcertificatedate);
                    while ($row = mysqli_fetch_array($querycertificaterequestdate)) {

                         $certificatedate = date("D M j Y g:i:s A", $row['unixdatetimecertificate']);
                    }
                    echo '<td>' . $certificatedate . '</td>';

                    $sqluserrole = "select mdl_role_assignments.userid, mdl_role_assignments.roleid,mdl_course_modules.course, mdl_role.shortname as rolename,FROM_UNIXTIME(mdl_role_assignments.timemodified)  from mdl_role_assignments 
        Inner Join
        mdl_context
        on
        mdl_context.id = mdl_role_assignments.contextid
        Inner Join
        mdl_course_modules
        on 
        mdl_course_modules.instance = mdl_context.instanceid
        Inner Join
        mdl_role
        on 
        mdl_role.id = mdl_role_assignments.roleid
        
        where mdl_course_modules.course = '" . $coursefilterid . "'
        and
        mdl_role_assignments.userid = '" . $userid . "'
        group by
        mdl_role_assignments.userid,
        mdl_role_assignments.roleid,
        mdl_course_modules.course,
        mdl_role.shortname,
        mdl_role_assignments.timemodified
        
        order by mdl_role_assignments.timemodified
        ";

                    $userlistrole = '';
                    $queryuserrole = mysqli_query($conn, $sqluserrole);
                    while ($row = mysqli_fetch_array($queryuserrole)) {

                        $userrole = $row['rolename'];
                        $userlistrole = array(
                            array(
                                $userrole
                            )
                        );
                        // echo '<td>'.$userrole.'</td>';
                    }

                    foreach ($userlistrole as $listrole) {

                        // echo $listrole;
                    }

                    $teacherrole = array(
                        'student'
                    );
                    foreach ($teacherrole as $rolename) {
                        $role = $DB->get_record('role', array(
                            'shortname' => $rolename
                        ));
                        $context = get_context_instance(CONTEXT_COURSE, $coursefilterid);
                        // $context = context_course::instance($cid1);
                        $teachers = get_role_users($role->id, $context);

                        foreach ($teachers as $teacher) {

                            $teacherid = $teacher->id;
                            if ($teacherid == $userid) {
                                echo '<td>student</td>';
                            }
                        }
                    }

                    $teacherrole = array(
                        'editingteacher'
                    );
                    foreach ($teacherrole as $rolename) {
                        $role = $DB->get_record('role', array(
                            'shortname' => $rolename
                        ));
                        $context = get_context_instance(CONTEXT_COURSE, $coursefilterid);
                        // $context = context_course::instance($cid1);
                        $teachers = get_role_users($role->id, $context);

                        foreach ($teachers as $teacher) {

                            $teacherid = $teacher->id;
                            if ($teacherid == $userid) {
                                echo '<td></td>';
                                echo '<td>editingteacher</td>';
                            }
                        }
                    }

                    $teacherrole = array(
                        'manager'
                    );
                    foreach ($teacherrole as $rolename) {
                        $role = $DB->get_record('role', array(
                            'shortname' => $rolename
                        ));
                        $context = get_context_instance(CONTEXT_COURSE, $coursefilterid);
                        // $context = context_course::instance($cid1);
                        $teachers = get_role_users($role->id, $context);

                        foreach ($teachers as $teacher) {

                            $teacherid = $teacher->id;
                            if ($teacherid == $userid) {
                                echo '<td>manager</td>';
                            }
                        }
                    }
                    echo '</tr>';
                }
            }

            ?>

    
        </tbody>
        <tfoot>

        </tfoot>
    </table>
</body>
</html>

<?php
    }
}

else {
    header("Location:/index.php");
    // echo "something";
    die();
}
}

else {

header("Location:/index.php");
die();

}
2

It’s quite difficult to read, but on the face of it you run a fairly large query, loop through the results, and inside the loop you then run some more queries to get additional information. It’s usually the case that you could add some JOINs to your original query and bring that information out in the original data set.

For example the first query you run inside the loop is almost as complex as the original query, retrieves lots of columns, but you only seem to use one. The comment suggests this is to retrieve the “module name”, though the variable is confusingly called “organization”, but as you’re already retrieving information from mdl_user-info_data in the first query, why not add the extra column to that instead of the new query?

The next one retrieves the user-id, which again comes from a table you’re already accessing in the first query, and even though it’s the user-id, the query to retrieve it is called $sqlCertificateDateuid - no big deal, but doesn’t make it easy to read. In fact now I read it again, you actually retrieve the user-id in the first query, so there’s really no need to get it again for every iteration of the loop.

Without going through the massive code dump, it does seem that there are some queries being run that could be combined.

3

  1. No SQL-params escaping.

  2. No HTML-escaping.

  3. Session in request body instead of cookie.

  4. HTTP-method POST by “show”-usecase.

  5. Are you sure, you should to show your CSV in HTML-format instead of just to download it with readfile()?

4

@djohnstone ,

Do you know how to use functions and their return values? The following script could be simplified:

<?php declare(strict_types=1);

$queryenrolleduser = getUser($conn=null);

//=============================
// usage:
// $queryenrolleduser = getUser($conn);
//==================/===========
function getUser
(
$conn=null,
$host='host', 
$username='username', 
$password='Pword',
$database='dbname'
)
{
$coursefilterid = $_GET['course'] ?? 'DEFAULT_COURSE';
$conn = new mysqli($host, $username, $password, $database);
if ($conn->connect_error) 
{
  die("Connection failed: " . $conn->connect_error);
 }
 $sqluserenrolled = "select mdl_user.username, mdl_user_enrolments.userid as enrolleduserid, mdl_enrol.courseid from mdl_user_enrolments
    Inner Join
    mdl_enrol
    on 
    mdl_enrol.id = mdl_user_enrolments.enrolid
    Inner Join
    mdl_user
    on
    mdl_user.id = mdl_user_enrolments.userid
    where  
    mdl_enrol.courseid = '" . $coursefilterid . "'
    order by mdl_user.username   
    ";
 $queryenrolleduser = mysqli_query($conn, $sqluserenrolled);

return $queryenrolleduser;
}///endfunc
5

Really appreciate everyone’s help with this. This was not my code, but had trouble trying to get my head around what the previous developer did. Great site!

1 Like
6

This topic was automatically closed 91 days after the last reply. New replies are no longer allowed.