1
\$\begingroup\$

I was looking at this website:

http://www.devshed.com/c/a/MySQL/Creating-User-Models-in-PHP-and-MySQL/1/ and http://www.devshed.com/c/a/MySQL/Creating-User-Models-in-PHP-and-MySQL/2/

In the first one they have methods such as:

/**
     * Perform a SELECT statement
     */ 
    public function select($table, $where = '', $fields = '*', $order = '', $limit = null, $offset = null)
    {
        $query = 'SELECT ' . $fields . ' FROM ' . $table
               . (($where) ? ' WHERE ' . $where : '')
               . (($limit) ? ' LIMIT ' . $limit : '')
               . (($offset && $limit) ? ' OFFSET ' . $offset : '')
               . (($order) ? ' ORDER BY ' . $order : '');
        $this->query($query);
        return $this->countRows();
    }

    /**
     * Perform an INSERT statement
     */  
    public function insert($table, array $data)
    {
        $fields = implode(',', array_keys($data));
        $values = implode(',', array_map(array($this, 'quoteValue'), array_values($data)));
        $query = 'INSERT INTO ' . $table . '(' . $fields . ')' . ' VALUES (' . $values . ')';
        $this->query($query);
        return $this->getInsertId();
    }

then in the second page:

/**
     * Fetch all users
     */
    public function fetchAll()
    {
        // try to fetch users data from the cache system
        if ($users = $this->_cache->get('all')) {
            return $users;
        }
        // otherwise fecth users data from the database
        $this->_adapter->select('users');
        $users = array();
        while ($user = $this->_adapter->fetch()) {
            $users[] = $user;
        }
        if (count($users) !== 0) {
           $this->_cache->set('all', $users); 
        }
        return $users;
    }

    /**
     * Insert new user data (the cache is cleared also)
     */
    public function insert(array $data)
    {
        if ($insertId = $this->_adapter->insert('users', $data)) {
            $this->_cache->clear();
            return $insertId;
        }
        return false;
    }

    /**
     * Delete the table row corresponding to the specified user (the cache is cleared also)
     */ 
    public function delete($id)
    {
        if ($affectedRow = $this->_adapter->delete('users', "id = $id")) {
            $this->_cache->clear();
            return $affectedRow; 
        }
        return false;
    }  

Is this a bad way of doing things? Is this opening me up to sql injection just like anything else where I construct the sql on the fly would? I want to build my models "right." Does that mean I need to have all stored procedures or prepared statements?

\$\endgroup\$

migrated from stackoverflow.com Aug 15 '12 at 13:27

This question came from our site for professional and enthusiast programmers.

  • \$\begingroup\$ possible duplicate of Best way to prevent SQL Injection in PHP \$\endgroup\$ – vascowhite Aug 14 '12 at 20:23
  • \$\begingroup\$ That's not a duplicate. I'll edit the title of this one to clarify. \$\endgroup\$ – Robert Harvey Aug 14 '12 at 20:24
  • \$\begingroup\$ I can't see where your parameters are coming from, but theoretically as long as you are escaping them properly you should be ok. \$\endgroup\$ – Wug Aug 14 '12 at 20:25
  • 4
    \$\begingroup\$ Don't do it this way, better to use PDO and parameterise your queries. There is a good tutorial here wiki.hashphp.org/PDO_Tutorial_for_MySQL_Developers. Also take a look at the information in the link in my previous comment. \$\endgroup\$ – vascowhite Aug 14 '12 at 20:26
  • \$\begingroup\$ @RobertHarvey You may be right, but there's still some good information there for the OP. In fact, I think the accepted answer there could answer this question. Don't you? \$\endgroup\$ – vascowhite Aug 14 '12 at 20:27
1
\$\begingroup\$

Active record instances are not "models". They are anti-patterns which RoR likes call that way. And what you are implementing there is an active record.

And no, those queries are not safe. They are wide open for SQL injection, because, instead of using proper SQL prepared statements, you opted for concatenation.

As for build building model, you have to understand that model is a layer. One of two in MVC. The other being - presentation layer (made of controllers, views and templates). You might read this answer, but I am no sure how much of it you will be able to assimilate.

I think your first priority should be to read the hashphp PDO tutorial and then watching the list if lecture that you can find a the bottom of this post. And only then try another stab at the whole "mvc thing".

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.