Entity::errors() will append errors and will merge the new given errors #840

Merged
merged 1 commit into from Mar 18, 2013

Conversation

Projects
None yet
4 participants
Member

rapzo commented Feb 27, 2013

with the previous ones already stored in the Entity's $_errors variable.

@nateabele nateabele commented on an outdated diff Feb 27, 2013

data/Entity.php
}
if ($value === null && isset($this->_errors[$field])) {
return $this->_errors[$field];
}
if ($value !== null) {
+ if (array_key_exists($field, $this->_errors)) {
+ $current = is_array($this->_errors[$field]) ?
@nateabele

nateabele Feb 27, 2013

Owner

If this is breaking over multiple lines, it should probably just be an if block.

Owner

nateabele commented Feb 27, 2013

Hey, looks like one of the new tests is broken.

Member

rapzo commented Feb 27, 2013

Yeah... My bad.. Didn't run all tests so, there were places where it was expected not to append errors but to get the last error. Thanks Travis o/

Member

rapzo commented Feb 28, 2013

Attaboy Travis!
Hope everything is ok.
One thing is not covered... Repeated messages... Tried some tricks to cover it but, at the end of the day, it should be developer's fault if it happens, right? In the worst case scenario will make look for repeated code, imho.

Contributor

jails commented Feb 28, 2013

Is there a way for clearing all errors ?

Owner

nateabele commented Feb 28, 2013

Hmm, I thought it was possible to pass false to reset, but it looks like no. I wonder how this hasn't caused other tests to fail.

On Feb 27, 2013, at 20:23, Simon JAILLET notifications@github.com wrote:

I there a way for clearing all errors ?


Reply to this email directly or view it on GitHub.

Member

rapzo commented Feb 28, 2013

There is no reset functionality. I thought about adding what @nateabele said because of the explicit null check (maybe it was on a lost todo list or something) but i confess i never saw it implemented and a quick history search proved. But if you want it, i'm your man :-D

Member

rapzo commented Feb 28, 2013

Squashed the reset trigger.
Travis likes. If you guys think this is dangerous or something it can to the shelf and meanwhile i'll test it on an app and shout some feedback.

@jails jails commented on an outdated diff Feb 28, 2013

data/Entity.php
}
if ($value === null && isset($this->_errors[$field])) {
return $this->_errors[$field];
}
if ($value !== null) {
+ if (array_key_exists($field, $this->_errors)) {
+ $current = $this->_errors[$field];
+ if (!is_array($current)) {
+ $current = array($current);
+ }
+ if (!is_array($value)) {
+ $value = array($value);
+ }
+ return ($this->_errors[$field] = Set::merge($current, $value));
@jails

jails Feb 28, 2013

Contributor

ẁith return ($this->_errors[$field] = Set::merge((array) $current, (array) $value)); you don't need the if (!is_array(..)) above imo.

@jails

jails Feb 28, 2013

Contributor

Is Set::merge is realy needed here ? array_merge is not enough ?

@jails jails commented on an outdated diff Feb 28, 2013

data/Entity.php
* @param string $value Value to set.
* @return mixed Either the `$this->_errors` array, or single value from it.
*/
public function errors($field = null, $value = null) {
+ if ($field === false) {
+ $this->_errors = array();
@jails

jails Feb 28, 2013

Contributor

You miss the return statement somewhere imo. return $this->_errors = array(); ?

Contributor

jails commented Feb 28, 2013

Imo it need at least a $this->errors(false); in Model::validates(). Weird any test fail. Imo it need an aditionnal test which test a failing validates then updating the entity to make the validates pass and checking if ::errors() returns really an empty array.

Member

rapzo commented Feb 28, 2013

Thanks for the input @jails o/ those a couple of lame examples of lazy development...
Will fix that soon and add a ton of tests to the model. Is it ok to inline add a filter for validates or should i create a specific mock for this?
I have this issue when validating arrays in a model, since there isn't a proper way for it i was filtering Model::validates() and had to check for errors and append them manually so the entity carried all the given errors. So, it would be nice to test it with a filter :-) what do you think?

Contributor

jails commented Feb 28, 2013

Woos me with beautiful code @rapzo and I'll tell you if I'm attracted ;-)

Member

rapzo commented Mar 1, 2013

Squashed.
Turned out to be the ugliest test case and maybe it's misplaced. Waiting for feedback.

Contributor

jails commented Mar 2, 2013

When you write unit tests @rapzo, you need to write tests only for one function logic at a time. And when possible split each tests to independant functions.

If I understand well this should be the tests I would do for Entity::errors()

public function testErrors() {
    $entity = new Entity();
    $errors = array('foo' => 'Something bad happened.');
    $this->assertEqual(array(), $entity->errors());

    $entity->errors($errors);
    $this->assertEqual($errors, $entity->errors());
    $this->assertEqual('Something bad happened.', $entity->errors('foo'));

    $errors += array('bar' => 'Something really bad happened.');
    $entity->errors($errors);
    $this->assertEqual($errors, $entity->errors());

    $this->assertCount(2, $entity->errors());
    $this->assertEqual('Something bad happened.', $entity->errors('foo'));
    $this->assertEqual('Something really bad happened.', $entity->errors('bar'));

    $entity->errors(false);
    $this->assertEmpty($entity->errors());
}

public function testAppendingErrors() {
    $entity = new Entity();
    $expected = array(
        'Something bad happened.',
        'Something really bad happened.'
    );

    $entity->errors('foo', $expected[0]);
    $entity->errors('foo', $expected[1]);

    $this->assertCount(1, $entity->errors());
    $this->assertEqual($expected, $entity->errors('foo'));
}

public function testAppendingErrorsWithArraySyntax() {
    $entity = new Entity();
    $expected = array(
        'Something bad happened.',
        'Something really bad happened.'
    );

    $entity->errors(array('foo' => $expected[0]));
    $entity->errors(array('foo' => $expected[1]));

    $this->assertCount(1, $entity->errors());
    $this->assertEqual($expected, $entity->errors('foo'));
}

public function testAppendingErrorsWithMixedSyntax() {
    $entity = new Entity();
    $expected = array(
        'Something bad happened.',
        'Something really bad happened.'
    );

    $entity->errors('foo', $expected[0]);
    $entity->errors(array('foo' => $expected[1]));

    $this->assertCount(1, $entity->errors());
    $this->assertEqual($expected, $entity->errors('foo'));
}

And to check errors are correctly cleared beetween two validates, this is the test I would do:

public function testErrorsIsClearedOnEachValidates() {
    $post = MockPostForValidates::create(array('title' => 'new post'));
    $result = $post->validates();
    $this->assertFalse($result);
    $result = $post->errors();
    $this->assertNotEmpty($result);

    $post->email = 'contact@lithify.me';
    $result = $post->validates();
    $this->assertTrue($result);
    $result = $post->errors();
    $this->assertEmpty($result);
}

A lot of above tests will fail imo. But maybe I don't understand the behavior you're expecting for Entity::errors() with this PR.

Member

rapzo commented Mar 2, 2013

WOW! Now this is something!to dwell about.
Thanks for the insights and the code. This enlightened how a behavior should be tested. I'll work with it and see if it is worth pulling this proposal.
Thanks again man, you guys make it a pleasure for us to try to participate.

Owner

gwoo commented Mar 2, 2013

The validates test should be in the ModelTest. Entity should only test that it can set,get,reset errors.

On Mar 1, 2013, at 7:23 PM, Simon JAILLET notifications@github.com wrote:

When you write unit tests @rapzo, you need to write tests only for one function logic at a time. And when possible split each tests to independant functions.

If I understand well what you're expecting this should be the tests I will go for the Entity::errors()

public function testErrors() {
$entity = new Entity();
$errors = array('foo' => 'Something bad happened.');
$this->assertEqual(array(), $entity->errors());

$entity->errors($errors);
$this->assertEqual($errors, $entity->errors());
$this->assertEqual('Something bad happened.', $entity->errors('foo'));

$errors += array('bar' => 'Something really bad happened.');
$entity->errors($errors);
$this->assertEqual($errors, $entity->errors());

$this->assertCount(2, $entity->errors());
$this->assertEqual('Something bad happened.', $entity->errors('foo'));
$this->assertEqual('Something really bad happened.', $entity->errors('bar'));

}

public function testAppendingErrors() {
$entity = new Entity();
$expected = array(
'Something bad happened.',
'Something really bad happened.'
);

$entity->errors('foo', $expected[0]);
$entity->errors('foo', $expected[1]);

$this->assertCount(1, $entity->errors());
$this->assertEqual($expected, $entity->errors('foo'));

}

public function testAppendingErrorsWithArraySyntax() {
$entity = new Entity();
$expected = array(
'Something bad happened.',
'Something really bad happened.'
);

$entity->errors(array('foo' => $expected[0]));
$entity->errors(array('foo' => $expected[1]));

$this->assertCount(1, $entity->errors());
$this->assertEqual($expected, $entity->errors('foo'));

}

public function testAppendingErrorsWithMixedSyntax() {
$entity = new Entity();
$expected = array(
'Something bad happened.',
'Something really bad happened.'
);

$entity->errors('foo', $expected[0]);
$entity->errors(array('foo' => $expected[1]));

$this->assertCount(1, $entity->errors());
$this->assertEqual($expected, $entity->errors('foo'));

}
And to check errors are correctly cleared beetween two validates, this is the test I will do:

public function testErrorsIsClearedOnEachValidates() {
$post = MockPostForValidates::create(array('title' => 'new post'));
$result = $post->validates();
$this->assertFalse($result);

$post->email = 'contact@lithify.me';
$result = $post->validates();
$this->assertTrue($result);
$result = $post->errors();
$this->assertEmpty($result);

}
And a lot of above tests will fail imo. But maybe I didn't understand the behavior you're expecting for Entity::errors() with this PR.


Reply to this email directly or view it on GitHub.

Owner

nateabele commented Mar 4, 2013

Hey @rapzo, sorry for all the back and forth. :-P Just ping me when you've incorporated those last two comments.

Member

rapzo commented Mar 13, 2013

Sorry for the long delay fellaz, i've been flooded and to be honest didn't have one opportunity to pick this up in the last 15 days.
Used the tests @jails posted (thanks again man!) and now everything seems ok.
Not sure if this breaks compatibility, at least for me it does since i use Entity::errors() a lot and for every time i checked if there was errors and appended new ones i got repeated errors. But maybe it was the way i did things hehe.

P.S.: rebased to current dev but now that i think of it maybe it wasn't such a good idea... If it screws history i'll close this one and will make a new PR

@jails jails and 1 other commented on an outdated diff Mar 13, 2013

tests/cases/data/ModelTest.php
+ 'email' => 'cheese@toast.com'
+ ));
+
+ $errors = array('cheesy title');
+ $post->errors('title', $errors[0]);
+ $this->assertEqual(array('title' => 'cheesy title'), $post->errors());
+
+ $errors[] = 'not good good';
+ $post->errors(array('title' => $errors[1]));
+ $this->assertCount(2, $post->errors('title'));
+ $this->assertEqual($errors, $post->errors('title'));
+
+ $this->assertTrue($post->validates());
+
+ $expected = array('title' => $errors);
+ $this->assertEqual($expected, $post->errors());
@jails

jails Mar 13, 2013

Contributor

Once validates() pass, errors() should returns an empty array.

@rapzo

rapzo Mar 13, 2013

Member

Erm, that's the thing. Should Entity::errors() be that flat for Model::validates()? Because if we take a look at the Model::validates()

if ($errors = $validator::check($entity->data(), $rules, $options)) {
    $entity->errors($errors);
}
return empty($errors);

Reseting the array would defeat the only thing, don't you think?

@jails

jails Mar 13, 2013

Contributor

Not sure to understand what you mean.

@rapzo

rapzo Mar 13, 2013

Member

Since Model::validates() only check for errors from checking the rules defined in Model::$validates it should erase any errors added previously to validation? IMO it should only check if its valid for the current defined rules.
For example, I find this useful specially when validating nested documents.

@jails jails commented on the diff Mar 13, 2013

tests/cases/data/EntityTest.php
@@ -118,6 +118,71 @@ public function testErrors() {
$entity->errors($errors);
$this->assertEqual($errors, $entity->errors());
$this->assertEqual('Something bad happened.', $entity->errors('foo'));
+
+ $otherError = array('bar' => 'Something really bad happened.');
+ $errors += $otherError;
+ $entity->errors($otherError);
+ $this->assertEqual($errors, $entity->errors());
+
+ $this->assertCount(2, $entity->errors());
+ $this->assertEqual('Something bad happened.', $entity->errors('foo'));
+ $this->assertEqual('Something really bad happened.', $entity->errors('bar'));
+ }
+
+ public function testResetErrors() {
@jails

jails Mar 13, 2013

Contributor

Well done forgeted that one

@jails jails commented on an outdated diff Mar 13, 2013

tests/cases/data/ModelTest.php
@@ -634,6 +634,53 @@ public function testValidationInheritance() {
$this->assertEqual($errors, $antique->errors());
}
+ public function testValidationWithPrependedErrors() {
@jails

jails Mar 13, 2013

Contributor

Imo this test is not needed and is not really a unit test.

@jails jails and 1 other commented on an outdated diff Mar 13, 2013

tests/cases/data/ModelTest.php
+
+ $this->assertTrue($post->validates());
+
+ $expected = array('title' => $errors);
+ $this->assertEqual($expected, $post->errors());
+
+ $post->email = null;
+ $expected += array('email' => array(
+ 'email is empty',
+ 'email is not valid'
+ ));
+ $this->assertFalse($post->validates());
+ $this->assertEqual($expected, $post->errors());
+ }
+
+ public function testValidationWithAppendedErrors() {
@jails

jails Mar 13, 2013

Contributor

Don't understand this one is it really needed ?

@rapzo

rapzo Mar 13, 2013

Member

Aaaah, i separated the method and shouldn't (as you commented above) to try to show explicitly the bonus of having this hehe.

@jails

jails Mar 14, 2013

Contributor

Oh ok just understand... this test is not needed, appending errors is already tested in EntityTest ;-).

Contributor

jails commented Mar 13, 2013

Imo you can safely replace added tests in ModelTest by the above testErrorsIsClearedOnEachValidates(). This test insure errors is cleared on each validates() and imo it's the only one test needed here.

Of course to make it pass you should add $entity->errors(false); in Model::validates(). Imo appending errors continuously can't work without some cleaning somewhere ;-).

@jails jails commented on an outdated diff Mar 14, 2013

tests/cases/data/ModelTest.php
@@ -634,6 +634,56 @@ public function testValidationInheritance() {
$this->assertEqual($errors, $antique->errors());
}
+ public function testErrorsIsClearedOnEachValidates() {
+ $post = MockPostForValidates::create(array(
+ 'title' => 'cheese',
+ 'email' => 'cheese@toast.com'
+ ));
+
+ $errors = array('cheesy title');
+ $post->errors('title', $errors[0]);
+ $this->assertEqual(array('title' => 'cheesy title'), $post->errors());
@jails

jails Mar 14, 2013

Contributor

You don't need to tests appended errors here since it has been tested in EntityTest.

@jails jails commented on an outdated diff Mar 14, 2013

tests/cases/data/ModelTest.php
@@ -634,6 +634,56 @@ public function testValidationInheritance() {
$this->assertEqual($errors, $antique->errors());
}
+ public function testErrorsIsClearedOnEachValidates() {
+ $post = MockPostForValidates::create(array(
+ 'title' => 'cheese',
+ 'email' => 'cheese@toast.com'
+ ));
+
+ $errors = array('cheesy title');
+ $post->errors('title', $errors[0]);
+ $this->assertEqual(array('title' => 'cheesy title'), $post->errors());
+
+ $errors[] = 'not good good';
+ $post->errors(array('title' => $errors[1]));
+ $this->assertCount(2, $post->errors('title'));
+ $this->assertEqual($errors, $post->errors('title'));
@jails

jails Mar 14, 2013

Contributor

Same here, you should test only one logic at a time and "appended errors" has already been tested in your EntityTest.

@rapzo rapzo Entity::errors() will append errors and will merge the new given errors
with the previous ones already stored in the Entity's $_errors variable.
Also, added a reset functionality to clear all errors.

Entity errors are cleared before the Model::validation() filter function to prevent validation (and consequent write data) with a not empty errors array.
Removed &$self from the filter since it was not used.
ixed tests.

Replacing tests with a more simple, direct approach (from Jails).
30c4154
Contributor

jails commented Mar 14, 2013

This one is fine for me ;-) !

Member

rapzo commented Mar 17, 2013

Is everything ok here? Do you guys need me to do anything else?

@nateabele nateabele added a commit that referenced this pull request Mar 18, 2013

@nateabele nateabele Merge pull request #840 from rapzo/feature/entity-appending-errors
Entity::errors() will append errors and will merge the new given errors
9e6f03a

@nateabele nateabele merged commit 9e6f03a into UnionOfRAD:dev Mar 18, 2013

1 check passed

default The Travis build passed
Details
Owner

nateabele commented Mar 18, 2013

Merged! Thanks for the prod. :-)

rapzo deleted the rapzo:feature/entity-appending-errors branch Mar 18, 2013

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