Is it bad practice to use variable variables in PHP?

Question

For example, a simple MVC type system:

/api/class/method rewritten into PHP variables using .htaccess/nginx.conf, then doing something like:

<?php

// Set up class + method variables
$className = some_class_filter($_GET['class']);
$method = some_method_filter($_GET['method']);

// Check if class exists and execute
if(file_exists(BASE . "/controllers/" . $className . ".class.php")) {
    require BASE . "/controllers/" . $className . ".class.php";
    $$className = new $className();

    // Execute the method
    $$className->$method();
} else {
    // Spit out some error based on the problem
}

?>

Is this horribly bad practice? If it is bad practice, can someone explain exactly why? And if so, is there a better way to do what I'm doing?

EDIT Essentially the reason I'm using variable variables is to make it simple to expand the core system - i.e. - adding in a new controller is nice and simple. I definitely understand the security risks of allowing essentially any function or class to be instantiated without some kind of filter.

The 'some_filter_here' could be a list of controllers that are allowed - whitelist as some here have mentioned.

Variable variables are pretty much always a bad idea. How about an array instead? http://stackoverflow.com/a/1817945/139010 — Matt Ball, Jan 03 '12 at 17:12
@LightnessRacesinOrbit - you are correct, thanks for pointing this out. — Jonathan Coe, Jan 03 '12 at 17:23

cmbuckley · Accepted Answer · 2012-01-03T17:25:49.750

Yes, this is rather bad practise. Do you need a variable variable for that instance? In other words, do you need more than one class & method to be instantiated in a given request? Your URI structure suggests not. If not, you could just use:

$object = new $className();
$object->$method();

Otherwise, you might want to do:

$objects = array();
$objects[$className] = new $className();
$objects[$className]->$method();

This avoids polluting the scope with variable variables, which are harder to track.

As far as the existence checks for your class in a given directory, this should be a sufficient whitelist (presuming an attacker cannot write to that directory).

EDIT: As a further check, you may want to consider checking method_exists on the object before calling the method.

I really like this, thanks for posting! I don't know why I didn't think of this originally. There are multiple objects being instantiated per request, although only one 'dynamic' class - although potentially more than one method is being called.. but multiple methods are easier to manage Thanks! — Jonathan Coe, Jan 03 '12 at 17:25

score 1 · Answer 2 · answered Jan 03 '12 at 17:14

1

Since you're writing the "some_class_filter" and "some_method_filter" code, I'd say it's OK. You also have a error or default handler I see, so in the end, I'd say it's alright.

I believe many MVC frameworks operate in a similar fashion anyway.

answered Jan 03 '12 at 17:14

Oomta

96
6

score 1 · Answer 3 · answered Jan 03 '12 at 17:28

They're not desirable, but it's fine to use them how you have.

A couple of pointers, though: your code does have a vulnerability where an attacker could traverse your directory with $_GET parameters like ?class=../base. If that file exists, your file_exists() call will return true and your application will attempt to include it and instantiate it as a class.

The safe scenario would be to whitelist those parameters to be letters, numbers and underscores only (if you separate words with underscores, i.e. .php).

Also, I prefer the syntax of using call_user_func and call_user_func_array. Using these functions in your code would look as follows:

<?php
$class_name = $_GET['class'];
$method_name = $_GET['method'];

$parameters = $_GET;
unset($parameters['class'], $parameters['method']); // grabs any other $_GET parameters

if (file_exists(BASE.'/controllers/'.$class_name.'.class.php')) {
    require BASE.'/controllers/'.$class_name.'.class.php';
    $controller = new $class_name();
    $response = call_user_func_array(array($controller, $action_name), $parameters);
}
else {
    header('HTTP/1.1 404 Not Found');
    // ...and display an error message
}

Thanks for the input! This makes a lot of sense, and i haven't used call_user_func/call_user_func_array before, i'll look into this more. — Jonathan Coe, Jan 03 '12 at 17:50

Is it bad practice to use variable variables in PHP?

3 Answers3

Linked