Adding an option builder for the save options #9143
Conversation
This will check associations and validation and throw exceptions if something is not set up right. This helps to prevent typos in the options and gives clear error messages for the developer.
Also added more doc blocks and tests.
| foreach ($array as $key => $value) { | ||
| $this->{$key}($value); | ||
| } | ||
| return $this; |
There was a problem hiding this comment.
Missing blank line before return statement
Current coverage is 94.90% (diff: 97.95%)@@ master #9143 diff @@
==========================================
Files 368 370 +2
Lines 27003 27084 +81
Methods 3233 3253 +20
Messages 0 0
Branches 0 0
==========================================
+ Hits 25640 25704 +64
- Misses 1363 1380 +17
Partials 0 0
|
|
I'm not sure |
|
@lorenzo how about simply set($key, $value) then and checking that no implemented method of the same name as $key exists? If it exists we could call it. |
| return $this->{$option}($value); | ||
| } | ||
| $this->_options[$option] = $value; | ||
| return $this; |
There was a problem hiding this comment.
Missing blank line before return statement
| { | ||
| $table = TableRegistry::get('Authors'); | ||
| $result = $table->getSaveOptionsBuilder(); | ||
| $this->assertInstanceOf('Cake\ORM\SaveOptionsBuilder', $result); |
There was a problem hiding this comment.
Could we have an integration test for the saveOptionBuilder() being used with save()?
There was a problem hiding this comment.
@markstory I'm not sure how to test this better. Any recommendations?
/**
* Test that a save call takes a SaveOptionBuilder object as well.
*
* @group save
* @return void
*/
public function testSaveWithOptionBuilder()
{
$table = $this->getMockBuilder('\Cake\ORM\Table')
->setMethods(['_processSave'])
->getMock();
$optionBuilder = new SaveOptionsBuilder($table, [
'associated' => []
]);
$entity = new \Cake\ORM\Entity(
['id' => 'foo'],
['markNew' => false, 'markClean' => true]
);
$this->assertSame($entity, $table->save($entity, $optionBuilder));
}There was a problem hiding this comment.
That looks good, I wouldn't use any mocks though. Set some options into the builder and check that the record and associations perhaps get saved as you'd expect.
| */ | ||
| public function validate($validate) | ||
| { | ||
| $this->_table->validator($validate); |
There was a problem hiding this comment.
Yes. This will mutate the table by building a validator and triggering the Model.buildValidator event if the validator hasn't already been built.
|
PHPCS builds are still failing. They'll need to be fixed before this can be merged. |
|
@burzum poke |
|
@lorenzo ping :) I'm still alive, I've been just busy with tournament preparations and other things than programming in my free time recently. |
|
@markstory @lorenzo is something missing? |
|
Thanks burzum! |
|
@burzum any chance you can add docs for this? |
|
This is very good stuff and all the option interfaces should move to setter methods or any other approach that is dependable / not-error prone. Thanks for doing it! Edit: @ypnos-web well not as long as there are no atoms, symbols, tuples/pattern-matching. Maybe we can move to class constants as keys (no idea if any good). |
|
@lorenzo when I find the time and mood to do so. Also I agree with @inoas. Maybe let's define an interface like \Cake\Core\OptionsInterface or an abstract class that will be used in the future (4.0) for all options passed to certain methods? I know this is a little more typing but I prefer to type a few chars more than ending up with wrong options because I had a typo in an array or having an option not checked. Also the default option values we have pretty often could be move into the options object itself. I had a nice debugging session because of that mistake and this was the reason for me to build this option builder. And since I'm in love with #c strict typed code feels more appealing to me. :) |
|
Let's face it, option arrays are not proper OO design, they are hard to document, hard to debug, and especially error-prone. |
| * | ||
| * @copyright Copyright (c) Cake Software Foundation, Inc. (http://cakefoundation.org) | ||
| * @link http://cakephp.org CakePHP(tm) Project | ||
| * @since 3.3.0 |
The option builder allows building the options through a chainable interface and does checks on some of the options to prevent mistakes like types in the associations. This helps the developer and ensures everything that is supposed to be saved gets really set up and saved right.