Browse Source

Don't invoke rules twice.

By decorating rules we can detect and prevent duplicate invocation of
rules. This issue was most easily observed with isUnique and existsIn
rules with custom errorField options. Previously those rules would emit
2 errors instead of one.
Mark Story 9 years ago
parent
commit
3852aa23c4

+ 140 - 0
src/Datasource/RuleInvoker.php

@@ -0,0 +1,140 @@
+<?php
+/**
+ * CakePHP(tm) : Rapid Development Framework (http://cakephp.org)
+ * Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
+ *
+ * Licensed under The MIT License
+ * For full copyright and license information, please see the LICENSE.txt
+ * Redistributions of files must retain the above copyright notice.
+ *
+ * @copyright     Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org)
+ * @link          http://cakephp.org CakePHP(tm) Project
+ * @since         3.2.12
+ * @license       http://www.opensource.org/licenses/mit-license.php MIT License
+ */
+namespace Cake\Datasource;
+
+/**
+ * Contains logic for invoking an application rule.
+ *
+ * Combined with Cake\Datasource\RuleChecker as an implementation
+ * detail to de-duplicate rule decoration and provide cleaner separation
+ * of duties.
+ *
+ * @internal
+ */
+class RuleInvoker
+{
+    /**
+     * The rule name
+     *
+     * @var string
+     */
+    protected $name;
+
+    /**
+     * Rule options
+     *
+     * @var array
+     */
+    protected $options = [];
+
+    /**
+     * Rule callable
+     *
+     * @var callable
+     */
+    protected $rule;
+
+    /**
+     * Constructor
+     *
+     * ### Options
+     *
+     * - `errorField` The field errors should be set onto.
+     * - `message` The error message.
+     *
+     * Individual rules may have additional options that can be
+     * set here. Any options will be passed into the rule as part of the
+     * rule $scope.
+     *
+     * @param callable $rule The rule to be invoked.
+     * @param string $name The name of the rule. Used in error messsages.
+     * @param array $options The options for the rule. See above.
+     */
+    public function __construct(callable $rule, $name, array $options = [])
+    {
+        $this->rule = $rule;
+        $this->name = $name;
+        $this->options = $options;
+    }
+
+    /**
+     * Set options for the rule invocation.
+     *
+     * Old options will be merged with the new ones.
+     *
+     * @param array $options The options to set.
+     * @return $this
+     */
+    public function setOptions(array $options)
+    {
+        $this->options = $options + $this->options;
+        return $this;
+    }
+
+    /**
+     * Set the rule name.
+     *
+     * Only truthy names will be set.
+     *
+     * @param string $name The name to set.
+     * @return $this
+     */
+    public function setName($name)
+    {
+        if ($name) {
+            $this->name = $name;
+        }
+        return $this;
+    }
+
+    /**
+     * Invoke the rule.
+     *
+     * @param \Cake\Datasouce\EntityInterface $entity The entity the rule
+     *   should apply to.
+     * @param array $scope The rule's scope/options.
+     * @return boolean Whether or not the rule passed.
+     */
+    public function __invoke($entity, $scope)
+    {
+        $rule = $this->rule;
+        $pass = $rule($entity, $this->options + $scope);
+        if ($pass === true || empty($this->options['errorField'])) {
+            return $pass === true;
+        }
+
+        $message = 'invalid';
+        if (isset($this->options['message'])) {
+            $message = $this->options['message'];
+        }
+        if (is_string($pass)) {
+            $message = $pass;
+        }
+        if ($this->name) {
+            $message = [$this->name => $message];
+        } else {
+            $message = [$message];
+        }
+        $errorField = $this->options['errorField'];
+        $entity->errors($errorField, $message);
+
+        if ($entity instanceof InvalidPropertyInterface && isset($entity->{$errorField})) {
+            $invalidValue = $entity->{$errorField};
+            $entity->invalid($errorField, $invalidValue);
+        }
+
+        return $pass === true;
+    }
+}

+ 6 - 27
src/Datasource/RulesChecker.php

@@ -312,32 +312,11 @@ class RulesChecker
             $name = null;
         }
 
-        return function ($entity, $scope) use ($rule, $name, $options) {
-            $pass = $rule($entity, $options + $scope);
-            if ($pass === true || empty($options['errorField'])) {
-                return $pass === true;
-            }
-
-            $message = 'invalid';
-            if (isset($options['message'])) {
-                $message = $options['message'];
-            }
-            if (is_string($pass)) {
-                $message = $pass;
-            }
-            if ($name) {
-                $message = [$name => $message];
-            } else {
-                $message = [$message];
-            }
-            $entity->errors($options['errorField'], $message);
-
-            if ($entity instanceof InvalidPropertyInterface && isset($entity->{$options['errorField']})) {
-                $invalidValue = $entity->{$options['errorField']};
-                $entity->invalid($options['errorField'], $invalidValue);
-            }
-
-            return $pass === true;
-        };
+        if (!($rule instanceof RuleInvoker)) {
+            $rule = new RuleInvoker($rule, $name, $options);
+        } else {
+            $rule->setOptions($options)->setName($name);
+        }
+        return $rule;
     }
 }

+ 50 - 0
tests/TestCase/ORM/RulesCheckerIntegrationTest.php

@@ -319,6 +319,30 @@ class RulesCheckerIntegrationTest extends TestCase
     }
 
     /**
+     * Ensure that add(isUnique()) only invokes a rule once.
+     *
+     * @return void
+     */
+    public function testIsUniqueRuleSingleInvocation()
+    {
+        $entity = new Entity([
+            'name' => 'larry'
+        ]);
+
+        $table = TableRegistry::get('Authors');
+        $rules = $table->rulesChecker();
+        $rules->add($rules->isUnique(['name']), '_isUnique', ['errorField' => 'title']);
+        $this->assertFalse($table->save($entity));
+
+        $this->assertEquals(
+            ['_isUnique' => 'This value is already in use'],
+            $entity->errors('title'),
+            'Provided field should have errors'
+        );
+        $this->assertEmpty($entity->errors('name'), 'Errors should not apply to original field.');
+    }
+
+    /**
      * Tests the isUnique domain rule
      *
      * @group save
@@ -419,6 +443,32 @@ class RulesCheckerIntegrationTest extends TestCase
     }
 
     /**
+     * Ensure that add(existsIn()) only invokes a rule once.
+     *
+     * @return void
+     */
+    public function testExistsInRuleSingleInvocation()
+    {
+        $entity = new Entity([
+            'title' => 'larry',
+            'author_id' => 500,
+        ]);
+
+        $table = TableRegistry::get('Articles');
+        $table->belongsTo('Authors');
+        $rules = $table->rulesChecker();
+        $rules->add($rules->existsIn('author_id', 'Authors'), '_existsIn', ['errorField' => 'other']);
+        $this->assertFalse($table->save($entity));
+
+        $this->assertEquals(
+            ['_existsIn' => 'This value does not exist'],
+            $entity->errors('other'),
+            'Provided field should have errors'
+        );
+        $this->assertEmpty($entity->errors('author_id'), 'Errors should not apply to original field.');
+    }
+
+    /**
      * Tests the existsIn domain rule when passing an object
      *
      * @group save