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

[NFR] Methods \Phalcon\Http\Request::setQueryFilters() and \Phalcon\Http\Request::getFilteredQuery() #13488

Closed
ekmst opened this Issue Sep 7, 2018 · 7 comments

Comments

Projects
3 participants
@ekmst
Copy link
Contributor

ekmst commented Sep 7, 2018

First I want to thank all participants of this wonderful project. Thank you for this project.

Type: new feature

I suggest adding methods : \Phalcon\Http\Request::setQueryFilters(string $name, array $filters) and \Phalcon\Http\Request::getFilteredQuery(string $name, $defaultValue = null, bool $notAllowEmpty = false, bool $noRecursive = false).

How to use:

// service registration
$di->setShared('request', function () {
        $request = new \Phalcon\Http\Request();
        
        // Global filters for the entire application
        $request->setQueryFilters('id', ['absint']);
        $request->setQueryFilters('role', ['trim', 'string', 'lower']);
        $request->setQueryFilters('param', ['absint']);

        return $request;
});

and in controllers:

class IndexController extends ControllerBase
{
    public function indexAction()
    {
        $param = $this->request->getFilteredQuery('param');
        // ....
    }
}

How did I realize this in my project:

<?php

namespace Demo\Components\Http;

class Request extends \Phalcon\Http\Request
{
    /**
     * @var array
     */
    protected $_queryFilters = [];

    /**
     * @param string $param
     * @param array  $filsters
     */
    public function setQueryFilters(string $name, array $filters)
    {
        $this->_queryFilters[$name] = $filters;
    }

    /**
     * @param string $name
     * @param null   $defaultValue
     * @param bool   $notAllowEmpty
     * @param bool   $noRecursive
     *
     * @return mixed
     */
    public function getFilteredQuery(string $name, $defaultValue = null, bool $notAllowEmpty = false, bool $noRecursive = false)
    {
        $filters = null;

        if (isset($this->_queryFilters[$name])) {
            $filters = $this->_queryFilters[$name];
        }

        return parent::getQuery($name, $filters, $defaultValue, $notAllowEmpty, $noRecursive);
    }
}

@ekmst ekmst closed this Sep 27, 2018

@wajdijurry

This comment has been minimized.

Copy link

wajdijurry commented Sep 28, 2018

+1

@ekmst ekmst reopened this Dec 22, 2018

@niden niden added the Enhancement label Dec 23, 2018

@niden niden self-assigned this Dec 23, 2018

@niden niden added this to To do in 4.0 Release via automation Dec 23, 2018

@niden niden moved this from To do to In progress in 4.0 Release Jan 28, 2019

@niden

This comment has been minimized.

Copy link
Member

niden commented Jan 28, 2019

@ekmst What would happen when the filter does not exist?

For instance consider this typo:

$di->setShared('request', function () {
        $request = new \Phalcon\Http\Request();
        
        // Global filters for the entire application
        $request->setQueryFilters('id', ['abbbbsint']);

        return $request;
});

Should we just return the value as is because the filter is not found or throw an exception? At the moment the filter locator throws an exception if it cannot locate the relevant sanitizer

I am in favor of leaving the exception because it can help with typos from the developer. Also if we are to check the sanitizers, then that will introduce another loop in finding if a sanitizer exists or not

@niden

This comment has been minimized.

Copy link
Member

niden commented Jan 28, 2019

BTW this should also be implemented in the rest methods getPost, getPut etc.

@niden

This comment has been minimized.

Copy link
Member

niden commented Jan 28, 2019

Also what happens if you try to call getFilteredQuery for a parameter that does not have filters defined? You get back the passed variable pretty much?

niden added a commit to niden/cphalcon that referenced this issue Jan 31, 2019

niden added a commit to niden/cphalcon that referenced this issue Feb 2, 2019

niden added a commit to niden/cphalcon that referenced this issue Feb 2, 2019

[phalcon#13488] - Merge branch '4.0.x' into T13488-get-query-filtered…
…-params

* 4.0.x:
  [4.0.x] - Added unicode flag for email filter
  [4.0.x] - Added unicode flag for email filter

niden added a commit to niden/cphalcon that referenced this issue Feb 2, 2019

@niden niden referenced this issue Feb 2, 2019

Merged

T13488 get query filtered params #13793

3 of 3 tasks complete

niden added a commit that referenced this issue Feb 2, 2019

niden added a commit that referenced this issue Feb 2, 2019

@niden

This comment has been minimized.

Copy link
Member

niden commented Feb 2, 2019

This has been implemented. Thank you @ekmst

@niden niden closed this Feb 2, 2019

4.0 Release automation moved this from In progress to Done Feb 2, 2019

niden added a commit to niden/cphalcon that referenced this issue Feb 2, 2019

[4.0.x] - Merge remote-tracking branch 'upstream/4.0.x' into 4.0.x
* upstream/4.0.x:
  [phalcon#13488] - Removed commas that crept in the .env file
  [phalcon#13488] - Minor corection to debug; Added funcitonality for getting filtered vars; Added tests
  [phalcon#13488] - Initial changes for the feature; adding methods

alexbusu pushed a commit to alexbusu/cphalcon that referenced this issue Feb 2, 2019

@ekmst

This comment has been minimized.

Copy link
Contributor Author

ekmst commented Feb 15, 2019

@ekmst What would happen when the filter does not exist?

For instance consider this typo:

$di->setShared('request', function () {
        $request = new \Phalcon\Http\Request();
        
        // Global filters for the entire application
        $request->setQueryFilters('id', ['abbbbsint']);

        return $request;
});

Should we just return the value as is because the filter is not found or throw an exception? At the moment the filter locator throws an exception if it cannot locate the relevant sanitizer

I am in favor of leaving the exception because it can help with typos from the developer. Also if we are to check the sanitizers, then that will introduce another loop in finding if a sanitizer exists or not

I like the exception option. I think it will be more correct.

@niden

This comment has been minimized.

Copy link
Member

niden commented Feb 15, 2019

I ended up throwing the exception when the sanitizer does not exist when you register them. This way you have an early warning that you made a typo rather than getting errors when the sanitization happens.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment