5
\$\begingroup\$

Below you can find the constructor method and related fields of the main class of my REST API (note that the class contains more code than displayed) written in PHP 7.1. It does various things with the HTTP-request. It starts with getting the arguments and then it gets the method in which the request is made (like POST or GET). The request gets trimmed of tags and that gets saved back into the instance/member variable $this->request.

Now, to my best knowledge, $this->request in PHP always points to the HTTP-request when a HTTP-request has just been made. The code can only get here when a HTTP-request has been made and $this->method has been filled ($this->method = $_SERVER['REQUEST_METHOD'];).

In other words, $this->method should always exist, but I keep getting the 'Field declared dynamically' warning:

enter image description here

Is it okay to just ignore the warning or is there a best practice to deal with it?

Other tips to improve my code are welcome too. I've 'learned' PHP back in version 4 (I used it for uni once). I struggle with the OO concepts since introduced in a language where so much is allowed.

class BaseAPI {

    protected $method = '';
    protected $endpoint = '';
    protected $verb = '';  
    protected $args = Array();  

    public function __construct($request)
    {
        header("Access-Control-Allow-Orgin: *");
        header("Access-Control-Allow-Methods: *");
        header("Content-Type: application/json");

        $this->args = explode('/', rtrim($request, '/'));
        $this->endpoint = array_shift($this->args);
        if (array_key_exists(0, $this->args) && !is_numeric($this->args[0])) {
            $this->verb = array_shift($this->args);
        }

        $this->method = $_SERVER['REQUEST_METHOD'];
        if ($this->method == 'POST' && array_key_exists('HTTP_X_HTTP_METHOD', $_SERVER)) {
            if ($_SERVER['HTTP_X_HTTP_METHOD'] == 'DELETE') {
                $this->method = 'DELETE';
            } else if ($_SERVER['HTTP_X_HTTP_METHOD'] == 'PUT') {
                $this->method = 'PUT';
            } else {
                throw new IncorrectHeaderException("Unexpected Header");
            }
        }

        switch ($this->method) {
            case 'DELETE':
            case 'PUT':
                // don't do anything yet
            case 'POST':
                $this->request = $this->_cleanInput($_POST);
                break;
            case 'GET':
                $this->request = $this->_cleanInput($_GET);
                break;
            default:
                throw new IncorrectMethodException('Invalid Method');
                break;
        }
    }

    private function _cleanInput($data)
    {
        $cleaned_input = Array();
        if (is_array($data)) {
            foreach ($data as $k => $v) {
                $cleaned_input[$k] = $this->_cleanInput($v);
            }
        } else {
            $cleaned_input = trim(strip_tags($data));
        }
        return $cleaned_input;
    }
}
\$\endgroup\$
4
\$\begingroup\$

Now, to my best knowledge, $this->request in PHP always points to the HTTP-request when a HTTP-request has just been made

My guess is that you are thinking of the super-global $_REQUEST? That is not the same as $this->request (unless somehow that instance variable gets set to something else)...

Is it okay to just ignore the warning or is there a best practice to deal with it?

Declare the property, just as the other instance properties are declared (i.e. $method, $endpoint, $verb, etc.):

class BaseAPI {

    protected $request;

That way the IDE will know that the property should at least exist for each instance (though it might not be assigned a value until that switch statement).

Other Feedback

Switch statement cases:

switch ($this->method) {
        case 'DELETE':
        case 'PUT':
            // don't do anything yet

Those first cases are missing a break; so the PHP pre-processor will fall-through to the POST case.

header() calls in constructor

The three calls to header() in the constructor feels a little strange but then again it is difficult to know what the rest of your code looks like. Typically those would go in a method to render/send output.

\$\endgroup\$
  • \$\begingroup\$ Would that not make $this->request save into the property instead of into the request? \$\endgroup\$ – Belle-Sophie Nov 21 '17 at 22:04
  • \$\begingroup\$ "instead of into the request"... I don't understand what that means... \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 21 '17 at 22:09
  • \$\begingroup\$ @Belle- oh - perhaps you are thinking of the super-global $_REQUEST? \$\endgroup\$ – Sᴀᴍ Onᴇᴌᴀ Nov 21 '17 at 22:11
  • \$\begingroup\$ I'm afraid that adding it as a field would make $this->request no longer behave as expected. But I've just tried it and it works. Thanks! I think I should probably delete those switch cases, since I'm not using them at the moment. They're literally useless. \$\endgroup\$ – Belle-Sophie Nov 21 '17 at 22:12
  • \$\begingroup\$ I think I might be thinking of the super-global $_REQUEST indeed. That seems logical. Some parts of php are magic to me, especially magic methods. \$\endgroup\$ – Belle-Sophie Nov 21 '17 at 22:16
3
\$\begingroup\$

I don't recommend emitting these headers so early, and also not as a side-effect of a constructor:

header("Access-Control-Allow-Orgin: *");
header("Access-Control-Allow-Methods: *");
header("Content-Type: application/json");

Headers should be emitted when formulating the response, not before the request has been parsed. Once headers have been emitted, you cannot reliably change the HTTP status code. You might want to send a redirect or a "no content" response.

In particular, I would handle both of your thrown exceptions such that they return HTTP status 405, "Method not allowed'.

\$\endgroup\$
  • \$\begingroup\$ Thanks, that seems like some good tips. I've included some more code. Should the headers go into _response() like I've done now? \$\endgroup\$ – Belle-Sophie Nov 22 '17 at 18:33
  • \$\begingroup\$ Please see What to do when someone answers. I have rolled back Rev 6 → 4. \$\endgroup\$ – 200_success Nov 22 '17 at 18:50
  • \$\begingroup\$ Oh :( okay, that's too bad \$\endgroup\$ – Belle-Sophie Nov 22 '17 at 18:53
  • \$\begingroup\$ You are welcome to ask a follow-up question as a separate post, though. \$\endgroup\$ – 200_success Nov 22 '17 at 18:54

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.