Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Schema casting #567

Closed
jails opened this issue Jul 9, 2012 · 5 comments
Closed

RFC: Schema casting #567

jails opened this issue Jul 9, 2012 · 5 comments

Comments

@jails
Copy link
Contributor

jails commented Jul 9, 2012

After some refactoring on data collection, it result the following in DocumentSet::_init() :

$this->_data = $schema->cast($this, $this->_data, compact('exists', 'pathKey'));
foreach ($this->_data as &$data) {
    $data = $schema->cast($this, $data, compact('pathKey', 'model'));
}

if we stick on tests, everything looks like working fine but it's kinda weird. DocumentSet need to manage two types of array.

Simple array :

array('4cb4ab6d7addf98506010000', '4cb4ab6d7addf98506010001');

And field based array :

array(
    array(
      'id' => '4cb4ab6d7addf98506010000'
    ),
   array(
      'id' => '4cb4ab6d7addf98506010001'
   )
)

To avoid edge effects what could be the best solution for this issue ? Reintroducing the DocumentArray with its own Document::__init() extending DocumentSet ? Or finding a way to dected passed datas to avoid having to use multiple classes ?

@nateabele
Copy link
Member

Yeah, this is why I added DocumentArray. ;-) Do you want to re-add DocumentArray and make DocumentSet extend it?

@jails
Copy link
Contributor Author

jails commented Jul 9, 2012

Yeah of course, but before I just wanted to show you this following refactoring of the schema casting :
jails@a209d15

Well if you don't like it, I'll re-add DocumentArray. Initially I tried to remove DocumentArray for the following reason.
When we deal with cascading relations you are intended to use the following syntax :

$object->myrelation = array(...);

myrelation can be an embeded relation but will also be a hasMany\hasOne or belongsTo relation. Since actually relations are not supported on MongoDB, the problem is not yet topical but the casting process will inspect the datas to make a choice beetween a DocumentSet, a DocumentArray or a Document instanciation. Imo this inspection will goes into a little more cumbersome process. Indeed the inspection will depend on the type of relation (if relevant), the pathkey and the type of the datas (it's a lot of check).

@nateabele
Copy link
Member

Okay understood. Yeah, the refactoring makes sense (I'm guessing it's a BC break because of the change to cast()'s signature?). So you're saying with this refactoring, we can avoid re-adding DocumentArray?

@jails
Copy link
Contributor Author

jails commented Jul 9, 2012

Yes the casting strategy in the above commit works on any type of data array without re-adding DocumentArray. Otherwise the BC break is not limited to the cast() 's signature. Indeed when wrap is set to true, the DocumentSchema->cast() will always return a Document or a DocumentSet (for making the recursivity casting works). You can see the difference in ExporterTest::testTypeCasting() where $result[] is now $result->.

The other "break" concern DocumentTest::testSetAndCoerceArray(). Before setting $doc->forceArray = false; returns an empty array. With this commit it will returns array('false') but I think tests will be clearer that my confused explanation ;-)

@nateabele
Copy link
Member

Yeah I think the fix makes a lot of sense. Having forceArray = false map to forceArray = array(false) seems like a pretty sensible solution.

nateabele added a commit that referenced this issue Jul 15, 2012
Fix `Schema::cast()` using according to #567
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants