Skip to content

Commit

Permalink
Make Query::addDefaultTypes() do what it says.
Browse files Browse the repository at this point in the history 
Instead of adding, addDefaultTypes() was replacing. While this is
useful, adding is more useful as we want the typemap to accumulate type
information throughout its lifetime.
  • Loading branch information
@markstory
markstory committed Jun 11, 2015
1 parent 3223358 commit 2400cf2
Show file tree
Hide file tree
Showing 2 changed files with 19 additions and 2 deletions.
19 changes: 18 additions & 1 deletion src/Database/TypeMap.php
View file
Expand Up @@ -64,6 +64,8 @@ public function __construct(array $defaults = [])
* $query->defaults(['created' => 'datetime', 'is_visible' => 'boolean']);
* ```
*
* This method will replace all the existing type maps with the ones provided.
*
* @param array $defaults associative array where keys are field names and values
* are the correspondent type.
* @return $this|array
Expand All @@ -78,7 +80,20 @@ public function defaults(array $defaults = null)
}

/**
* Configures a map of fields and their associated types for single-use.
* Add additional default types into the type map.
*
* If a key already exists it will not be overwritten.
*
* @param array $types The additional types to add.
* @return void
*/
public function addDefaults(array $types)
{
$this->_defaults = $this->_defaults + $types;
}

/**
* Sets a map of fields and their associated types for single-use.
*
* If called with no arguments it will return the currently configured types.
*
Expand All @@ -88,6 +103,8 @@ public function defaults(array $defaults = null)
* $query->types(['created' => 'time']);
* ```
*
* This method will replace all the existing type maps with the ones provided.
*
* @param array $types associative array where keys are field names and values
* are the correspondent type.
* @return $this|array
Expand Down
2 changes: 1 addition & 1 deletion src/ORM/Query.php
View file
Expand Up @@ -143,7 +143,7 @@ public function addDefaultTypes(Table $table)
foreach ($schema->columns() as $f) {
$fields[$f] = $fields[$alias . '.' . $f] = $schema->columnType($f);
}
$this->defaultTypes($fields);
$this->typeMap()->addDefaults($fields);

return $this;
}
Expand Down

3 comments on commit 2400cf2

@StewEucen
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markstory I have two confirms in line:92.

Confirm

(1) If $types has same keys with $this->_defaults, $this->_defaults will not be overwritten by $types.
Because left operand has a priority than right operand in "array + array".
If $types can overwrite same keys, must be as follow.

$this->_defaults = $types + $this->_defaults;

(2) When called addDefaults(), $this->_defaults is possible to be null.
Because it is not initialized in line:31.
Next code makes error, therefore line:92 has possibility to throw error.

$a = null + ['id' => 'integer'];
$a = ['id' => 'integer'] + null;

Error: Unsupported operand types

It is required to cast as array variables for avoiding error.
If can not happen that $this->_defaults and $types are null, it is not required to cast.

Suggestion

If (1) (2) is right, should be as follow

    public function addDefaults(array $types)
    {
---     $this->_defaults = $this->_defaults + $types;
+++     $this->_defaults = (array)$types + (array)$this->_defaults;
    }

If use to initialize on line:31, to cast array for $this->_defaults is not required in my code.
However it makes very wide side effect.

--- protected $_defaults;
+++ protected $_defaults = [];

@markstory
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The _defaults array is initialized in the constructor so it should never be null.

I didn't want to overwrite existing types with addDefaults() because I saw the method as an additive process that should preserve the existing table keys.

Overwriting keys may cause issues where tables have columns of the same name but the types are different. I felt it was safer to stick with the first values in.

@StewEucen
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@markstory Thank you for reply.
I cleared it. It is very important to know the thinking of other creators for such this huge project.

Please sign in to comment.