Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Add cleanRecursive method to JInputFilter + getAll method to Jinput #1548

Merged
merged 2 commits into from

7 participants

@florianv

Some people inject directly all the data contained in $_POST in their models.

It was possible with JRequest to retrieve all the data from a global var, but not with JInput, so I added the getAll method.

I also added a doc comment for the 'hidden' properties, so the editor doesn't complain.

@dongilbert
Collaborator

I'd like to see some $_SERVER and $_FILES mock tests as well. And I know @pasamio or @realityking would probably ask you to squash these commits (they've asked me to do that a few times when there were minor changes between commits)

Otherwise, +1

libraries/joomla/input/input.php
@@ -165,6 +171,18 @@ public function get($name, $default = null, $filter = 'cmd')
}
/**
+ * Get all values from the input data.
+ *
+ * @return array The input data.
+ *
+ * @since 12.2
+ */
+ public function getAll()

I think there should be a filter option too - like get method has.

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

Yes, I did it too quickly, I added the filters now.

@ianmacl

My biggest concern with this is the overlap with getArray and the ambiguity of what to use. Also, you're duplicating functionality by putting the recursive clean in JFilterInput and in JInput::getArray. I also wonder how useful this is beyond getArray if everything either gets filtering to CMD or you have to supply a full associative array of keys. getArray already handles the full associative array of keys. Are there cases where you want all the variables but you are ok with them all being CMD? I'm not entirely sure.

@florianv

The sensible difference is that with getArray if you don't pass an array of keys, you don't get anything.
Also if no filter is specified, it goes to the default case in JFilterInput::clean.

But with getAll you get all keys filtered with the default cmd filter.

I did this pull request because I saw many times people doing $model->store(JRequest::get('post')).
And they don't move to JInput because they don't know they can do $this->input->post->getArray($_POST) to have the same result.

So adding the getAll method seemed necessary.

When I did the first implementation, I used getArray, but then I have been told to add the filters, and I thought : I have the data, I just need to filter it and return. So for me the most natural way was to add that functionnality to JFilterInput instead of using getArray recursion.

From a logical point of view, I have the data in the class property, but I have to pass it to a method in that class only to use his call to the filter class, that I could call directly didn't seem good, so I did it like that.

(But yes I could also have concluded this story in one line)

@joomla-jenkins
Collaborator

I guess my point is that it would seem to make sense to change the getArray() method to return everything CMD filtered if no parameter is supplied rather than creating a new method that does almost exactly the same thing.

@pasamio

I'd suggest that blindly accepting arguments and jamming them into your model then trying to persist them is a bad pattern to follow. Thus further encouraging bad practice is really not something I can get behind. Having to explicitly specify in your controller what you want to pull out of the untrusted request source and being very clear about the filtering you want to impose is something that the platform is in general trying to move towards. This is similar to the direction already taken with the new routers where the path is to be a bit more explicit about what is routed where.

@florianv

Yes, it is not good pracctice, but I saw it many times.

I updated the pull request, see diff.
I think this a fair alternative regarding what @ianmacl and @joomla-jenkins said.

I didn't squash them to keep cleanRecursive, just in case.

@LouisLandry LouisLandry merged commit b157f28 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 10, 2012
  1. @florianv
Commits on Oct 13, 2012
  1. @florianv

    Add default case to getArray

    florianv authored
This page is out of date. Refresh to see the latest.
View
15 libraries/joomla/input/input.php
@@ -18,6 +18,12 @@
* @subpackage Input
* @since 11.1
*
+ * @property-read JInput $get
+ * @property-read JInput $post
+ * @property-read JInput $server
+ * @property-read JInputFiles $files
+ * @property-read JInputCookie $cookie
+ *
* @method integer getInt() getInt($name, $default = null) Get a signed integer.
* @method integer getUint() getUint($name, $default = null) Get an unsigned integer.
* @method float getFloat() getFloat($name, $default = null) Get a floating-point number.
@@ -168,14 +174,21 @@ public function get($name, $default = null, $filter = 'cmd')
* Gets an array of values from the request.
*
* @param array $vars Associative array of keys and filter types to apply.
+ * If empty and datasource is null, all the input data will be returned
+ * but filtered using the default case in JFilterInput::clean.
* @param mixed $datasource Array to retrieve data from, or null
*
* @return mixed The filtered input data.
*
* @since 11.1
*/
- public function getArray(array $vars, $datasource = null)
+ public function getArray(array $vars = array(), $datasource = null)
{
+ if (empty($vars) && is_null($datasource))
+ {
+ $vars = $this->data;
+ }
+
$results = array();
foreach ($vars as $k => $v)
View
23 tests/suites/unit/joomla/input/JInputTest.php
@@ -285,6 +285,29 @@ public function testGetArrayNested()
}
/**
+ * Test the JInput::getArray method without specified variables.
+ *
+ * @return void
+ *
+ * @since 12.3
+ */
+ public function testGetArrayWithoutSpecifiedVariables()
+ {
+ $array = array(
+ 'var2' => 34,
+ 'var3' => array('var2' => 'test'),
+ 'var4' => array('var1' => array('var2' => 'test')),
+ 'var5' => array('foo' => array()),
+ 'var6' => array('bar' => null),
+ 'var7' => null
+ );
+
+ $input = new JInput($array);
+
+ $this->assertEquals($input->getArray(), $array);
+ }
+
+ /**
* Test the JInput::get method.
*
* @return void
Something went wrong with that request. Please try again.