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:
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;
}
}
2 Answers 2
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.
-
\$\begingroup\$ Would that not make
$this->request
save into the property instead of into the request? \$\endgroup\$Belle– Belle2017年11月21日 22:04:49 +00:00Commented Nov 21, 2017 at 22:04 -
\$\begingroup\$ "instead of into the request"... I don't understand what that means... \$\endgroup\$2017年11月21日 22:09:02 +00:00Commented Nov 21, 2017 at 22:09
-
-
\$\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– Belle2017年11月21日 22:12:27 +00:00Commented Nov 21, 2017 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– Belle2017年11月21日 22:16:35 +00:00Commented Nov 21, 2017 at 22:16
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'.
-
\$\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– Belle2017年11月22日 18:33:31 +00:00Commented Nov 22, 2017 at 18:33 -
\$\begingroup\$ Please see What to do when someone answers . I have rolled back Rev 6 → 4. \$\endgroup\$200_success– 200_success2017年11月22日 18:50:27 +00:00Commented Nov 22, 2017 at 18:50
-
\$\begingroup\$ Oh :( okay, that's too bad \$\endgroup\$Belle– Belle2017年11月22日 18:53:50 +00:00Commented Nov 22, 2017 at 18:53
-
\$\begingroup\$ You are welcome to ask a follow-up question as a separate post, though. \$\endgroup\$200_success– 200_success2017年11月22日 18:54:58 +00:00Commented Nov 22, 2017 at 18:54
Explore related questions
See similar questions with these tags.