Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Removing `Entity::_relationships` and refactoring `Entity::__isset` & `Entity::__unset` : WARNING BC BREAK. #572

Closed
wants to merge 1 commit into from

3 participants

Simon JAILLET Nate Abele Hans Donner
Simon JAILLET
Collaborator

Separating Entity::_relationships & Entity::_updated has imo not a good thing since they are both datas and currently such separation only work on the loading step. Indeed if I use the following syntax :

$entity->mydata = $somedata // the datas goes into _updated even if it's a relation

So I think it's preferable to remove this feature for the moment and rethink of it when it'll be really necessary.

Secondarily close #571

Nate Abele
Owner

Relationships are fully implemented for Record/RecordSet. They already work correctly for relational databases, and will soon work correctly for non-relational as well. So this patch completely breaks existing relationship support.

Simon JAILLET
Collaborator

Yeah relationships are fully implemented and imo it's not reliyed on the way datas are stored. In this PR $_relationship is removed and all test pass anyway. Moreover $_relationship is not representative of $_relationship where updates are done on $_update. Actually it's just merged to $_updated and don't bring any extra features, only extra checks. Imo relationships more rely on the way to extract and detect them easily when needed (this is the aim of #575). Here $_relationship are extracted on finds but not used.

Hans Donner hans-d commented on the diff
data/Entity.php
((5 lines not shown))
+ return isset($this->_updated[$name]);
+ }
+
+ /**
+ * PHP magic method used when unset() is called on an `Entity` instance.
+ * {{{
+ * $doc = Post::find($id);
+ * unset($doc->fieldName);
+ * $doc->save();
+ * }}}
+ *
+ * @param string $name The name of the field to unset.
+ * @return void
+ */
+ public function __unset($name) {
+ unset($this->_updated[$name]);
Hans Donner
hans-d added a note

should we do a unset(), or reset it to the value of $this->_data[$name] when available?

For NoSQL unset makes sense, for SQL it does not. For other datasources it may vary. (originally the unset is only present in Document)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Nate Abele nateabele closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jul 13, 2012
  1. Simon JAILLET

    Removing Entity::_relationships and refactoring Entity::__isset & Ent…

    jails authored
    …ity::__unset : WARNING BC BREAK.
This page is out of date. Refresh to see the latest.
31 data/Entity.php
View
@@ -43,14 +43,6 @@ class Entity extends \lithium\core\Object {
protected $_data = array();
/**
- * An array containing all related records and recordsets, keyed by relationship name, as
- * defined in the bound model class.
- *
- * @var array
- */
- protected $_relationships = array();
-
- /**
* If this record is chained off of another, contains the origin object.
*
* @var object
@@ -111,7 +103,7 @@ class Entity extends \lithium\core\Object {
*/
protected $_autoConfig = array(
'classes' => 'merge', 'parent', 'schema', 'data',
- 'model', 'exists', 'pathKey', 'relationships'
+ 'model', 'exists', 'pathKey'
);
/**
@@ -126,7 +118,7 @@ class Entity extends \lithium\core\Object {
* @return object Record object.
*/
public function __construct(array $config = array()) {
- $defaults = array('model' => null, 'data' => array(), 'relationships' => array());
+ $defaults = array('model' => null, 'data' => array());
parent::__construct($config + $defaults);
}
@@ -173,7 +165,22 @@ public function __set($name, $value = null) {
* @return mixed Result.
*/
public function __isset($name) {
- return isset($this->_updated[$name]) || isset($this->_relationships[$name]);
+ return isset($this->_updated[$name]);
+ }
+
+ /**
+ * PHP magic method used when unset() is called on an `Entity` instance.
+ * {{{
+ * $doc = Post::find($id);
+ * unset($doc->fieldName);
+ * $doc->save();
+ * }}}
+ *
+ * @param string $name The name of the field to unset.
+ * @return void
+ */
+ public function __unset($name) {
+ unset($this->_updated[$name]);
Hans Donner
hans-d added a note

should we do a unset(), or reset it to the value of $this->_data[$name] when available?

For NoSQL unset makes sense, for SQL it does not. For other datasources it may vary. (originally the unset is only present in Document)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
}
/**
@@ -414,8 +421,6 @@ public function to($format, array $options = array()) {
switch ($format) {
case 'array':
$data = $this->_updated;
- $rel = array_map(function($obj) { return $obj->data(); }, $this->_relationships);
- $data = $rel + $data;
$result = Collection::toArray($data, $options);
break;
case 'string':
6 data/collection/RecordSet.php
View
@@ -318,12 +318,12 @@ protected function _mapRecord($data) {
$data = $conn->item($relModel, $data, $options);
}
$opts = array('class' => 'set');
- $relationships[$field] = $conn->item($relModel, $rel, $options + $opts);
+ $main[$field] = $conn->item($relModel, $rel, $options + $opts);
continue;
}
- $relationships[$field] = $conn->item($relModel, $rel, $options);
+ $main[$field] = $conn->item($relModel, $rel, $options);
}
- return $conn->item($primary, $main, $options + compact('relationships'));
+ return $conn->item($primary, $main, $options);
}
protected function _columnMap() {
29 data/entity/Document.php
View
@@ -110,7 +110,7 @@ public function &__get($name) {
return $this->_getNested($name);
}
- if (isset($this->_embedded[$name]) && !isset($this->_relationships[$name])) {
+ if (isset($this->_embedded[$name])) {
throw new RuntimeException("Not implemented.");
}
$result = parent::__get($name);
@@ -249,33 +249,6 @@ protected function _setNested($name, $value) {
}
/**
- * PHP magic method used to check the presence of a field as document properties, i.e.
- * `$document->_id`.
- *
- * @param $name The field name, as specified with an object property.
- * @return boolean True if the field specified in `$name` exists, false otherwise.
- */
- public function __isset($name) {
- return isset($this->_updated[$name]);
- }
-
- /**
- * PHP magic method used when unset() is called on a `Document` instance.
- * Use case for this would be when you wish to edit a document and remove a field, ie.:
- * {{{
- * $doc = Post::find($id);
- * unset($doc->fieldName);
- * $doc->save();
- * }}}
- *
- * @param string $name The name of the field to remove.
- * @return void
- */
- public function __unset($name) {
- unset($this->_updated[$name]);
- }
-
- /**
* Allows several properties to be assigned at once.
*
* For example:
22 tests/cases/data/EntityTest.php
View
@@ -40,21 +40,31 @@ public function testPropertyIssetEmpty() {
$entity = new Entity(array(
'model' => 'Foo',
'exists' => true,
- 'data' => array('test_field' => 'foo'),
- 'relationships' => array('test_relationship' => array('test_me' => 'bar'))
+ 'data' => array('test_field' => 'foo')
));
$this->assertEqual('foo', $entity->test_field);
- $this->assertEqual(array('test_me' => 'bar'), $entity->test_relationship);
$this->assertTrue(isset($entity->test_field));
- $this->assertTrue(isset($entity->test_relationship));
$this->assertFalse(empty($entity->test_field));
- $this->assertFalse(empty($entity->test_relationship));
$this->assertTrue(empty($entity->test_invisible_field));
- $this->assertTrue(empty($entity->test_invisible_relationship));
+ }
+
+ public function testPropertyUnset() {
+ $entity = new Entity(array(
+ 'model' => 'Foo',
+ 'exists' => true,
+ 'data' => array('test_field' => 'foo')
+ ));
+
+ $this->assertEqual('foo', $entity->test_field);
+ unset($entity->test_field);
+
+ $this->assertFalse(isset($entity->test_field));
+
+ $this->assertTrue(empty($entity->test_field));
}
public function testIncrement() {
Something went wrong with that request. Please try again.