-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.x] Functions to access Request parameters with specific data types #17177
base: 5.x
Are you sure you want to change the base?
Conversation
These functions solve the problem with functions like Request->getQuery() that return a mixed type. Many programmers overlook the possibility of receiving an unexpected array instead of a string, which can occur, for example, when bots are searching for security vulnerabilities. In certain cases, script execution may fail due to input validation occurring deeper in the code. PHP is becoming more type-oriented, and tools like PhpStan encourage documenting mixed type through PhpDocs comments. These functions prevent potentially dangerous typecasting of mixed type, so we enhance the overall security. The development process becomes more efficient as programmers no longer need to focus on handling multiple input types. And no need to add comments/suppressions for Psalm and PhpStan. Before: ```php $token = $this->request->getCookie($this->_config['cookie']); if ($token !== null) { /** @psalm-suppress PossiblyInvalidCast */ $token = (string)$token; } return $token; ``` After: ```php return $this->request->getCookieString($this->_config['cookie']); ``` The new functions cover the "get" operations from: - $request->cookies - $request->query - $request->params - $request->data For types: - int - string - bool Q: Is the `$default` parameter missing? A: No, instead of `$default`, the `??` operator is now used, starting from PHP 7 :)
Thanks for putting this together. I like the idea of providing a type safe way to access request data. I'm a bit concerned with the number of methods we would be adding, and where we're adding them. While a trait makes it simple to compose this logic into components and controllers, I feel like we should be having this logic on the request itself. Perhaps we could use named parameters to help expand the existing $value = $this->request->getQuery('page', 1, as: 'int'); I think this interface could be more ergonomic, but is harder to have typing work out as PHP doesn't have a good way to narrow method return types. 🤔 |
@markstory Thanks for the good feedback ♥. We use that Trait in our work. Unfortunately, extending the ServerRequest itself outside of Cake is not possible, so we opted for using the Trait on Controllers instead. These methods are very popular :). I have another idea. What do you think about it? (I wrote it quickly without any checking.) /**
* @method string|null getCookieString(string $key, string|null $default = null)
* @method bool|null getCookieBool(string $key, bool|null $default = null)
* @method int|null getCookieInt(string $key, int|null $default = null)
* ...
*/
class ServerRequest implements ServerRequestInterface
{
public function __call(string $name, array $params): mixed
{
if (str_starts_with($name, 'is')) {
// ...
return $this->is(...$params);
} elseif (str_starts_with($name, 'getCookie')) {
return $this->resolveAsType($name, $this->cookies, ...$params);
} elseif (str_starts_with($name, 'getData')) {
return $this->resolveAsType($name, $this->data ?? [], ...$params);
} elseif (str_starts_with($name, 'getParam')) {
return $this->resolveAsType($name, $this->params, ...$params);
} elseif (str_starts_with($name, 'getQuery')) {
return $this->resolveAsType($name, $this->query, ...$params);
}
throw new BadMethodCallException(sprintf('Method `%s()` does not exist.', $name));
}
private function resolveAsType(string $methodName, array|object $source, mixed ...$args): mixed
{
$asType = str_replace(['getCookie', 'getQuery', 'getParam', 'getData'], '', $methodName);
$rawValue = Hash::get($source, $args[0], $args[1]);
return match ($asType) {
'String' => is_string($rawValue) ? $rawValue : null,
'Int' => filter_var($rawValue, FILTER_VALIDATE_INT, FILTER_NULL_ON_FAILURE),
'Bool' => $this->filterBool($rawValue),
default => throw new BadMethodCallException(sprintf('Method `%s()` does not exist.', $methodName)),
};
}
} |
@jozefgrencik That's an interesting approach. We had a related discussion in the core team and had another approach to consider as well: namespace Cake\Utility;
function toInt(?mixed $value): ?int
{
} Each type conversion would have a separate function that could be imported and used in both userland and framework code. |
@markstory Nice. I like it. I will create a new class with the toInt()/toBool.. functions in a separate PR, and then we will see how we proceed with this PR. Okay? |
Sounds great. Thank you 🙇 |
This PR introduces a set of methods for converting mixed values to string/int/bool data types, ensuring a type-safe approach to data manipulation. This utility is useful for safely narrowing down the data types. cakephp#17177 (comment) https://en.wikipedia.org/wiki/IEEE_754 https://www.h-schmidt.net/FloatConverter/IEEE754.html https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER
@markstory @ravage84 Hi guys, is there anything missing here from @jozefgrencik? or it can be merged. |
Hi. First, we need to resolve #17295, then we'll be able to continue. I'll take a look at the PR today. Thx. |
This pull request is stale because it has been open 30 days with no activity. Remove the |
Now that #17295 is merged I am happy to contribute to this branch to get it closer to done if @jozefgrencik doesn't mind. |
This PR introduces a set of methods for converting mixed values to string/int/bool data types, ensuring a type-safe approach to data manipulation. This utility is useful for safely narrowing down the data types. cakephp/cakephp#17177 (comment) https://en.wikipedia.org/wiki/IEEE_754 https://www.h-schmidt.net/FloatConverter/IEEE754.html https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/MAX_SAFE_INTEGER
@jamisonbryant There are multiple approaches mentioned in the comments and I am not sure which solution is the best. Maybe the core-team should agree on the solution first. |
So how do we proceed? Having more granular access similar to Symfony sure should be a useful addition to 5.next |
I have attached a set of utility classes that I have created for my own CakePHP projects. Example use case: $intOrNull = IntCast::tryFrom($this->getRequest()->getQuery('test'));//returns int or null
$int = IntCast::from($this->getRequest()->getQuery('test'));//returns int or throws exception
$stringOrNull = StringCast::tryFrom($this->getRequest()->getQuery('test'));//returns string or null
$string = StringCast::from($this->getRequest()->getQuery('test'));//returns string or throws exception |
I agree that instead of overloading the Request, this is the easier opt-in mechanism for now. |
I have reuploaded and replaced attachment in my previous post because StringCast was missing in previous zip |
These functions solve the problem with functions like Request->getQuery() that return a mixed type.
Many programmers overlook the possibility of receiving an unexpected array instead of a string, which can occur, for example, when bots are searching for security vulnerabilities. In certain cases, script execution may fail due to input validation occurring deeper in the code.
PHP is becoming more type-oriented, and tools like PhpStan encourage documenting mixed type through PhpDocs comments.
These functions prevent potentially dangerous typecasting of mixed type, so we enhance the overall security.
The development process becomes more efficient as programmers no longer need to focus on handling multiple input types. And no need to add comments/suppressions for Psalm and PhpStan.
Before:
After:
The new functions cover the "get" operations from:
For types:
Q: Is the
$default
parameter missing?A: No, instead of
$default
, the??
operator is now used, starting from PHP 7 :)