Wouldn't this make more sense?

[gwagner]

In Shanty_Mongo_Collection under the static one() and static all() functions, we are adding inheritance to the query. Wouldn't it make sense to query against the entire inheritance instead of just the top element, in case someone is subclassing and decided they want to break things up one last time, they wont loose records.

<?php
$inheritance = static::getCollectionInheritance();
if (count($inheritance) > 1) 
     $query['_type'] = $inheritance[0];

vs

<?php
$inheritance = static::getCollectionInheritance();
if (count($inheritance) > 1) 
     $query['_type'] = array('$in' => $inheritance);

Also, when we rename a class, we cant query against old records because they are no longer in the inheritance array(). For example i had a class called Model_Project_Mongo_ComponentClassA and i renamed it to Model_Project_Mongo_ClassA. Any query i make for anything from the ComponentClassA collection is now missing because i am querying against _type Model_Project_Mongo_ClassA when the records in the DB only have Model_Project_Mongo_ComponentClassA attached to them. We should have a config param like $_previousClass = array('Model_Project_Mongo_ComponentClassA') that we can merge into the inheritance array.

Thoughts?

[tholder]

I can see the reason for wanting this, I think it's an inherent problem with Mongo really, interesting to know how other libs solve it? (if at all).

Whilst actually I think your code is probably cleaner, would it not be better to just have a migration tool to find references to the previous class in _type fields and update? You'll be constantly carrying around legacy references otherwise.

[gwagner]

Mongo doesn't care about _type ever, the driver doesn't care either. This is purely a shanty mongo problem.

You would be carrying around legacy references until the user writes a process to turn over the db, but that shouldn't be the job of the driver, and if a user wants to carry old references forever, then so be it. The feature should be there regardless.

Sent from my iPhone

On Mar 26, 2012, at 5:47 PM, Tom Holder reply@reply.github.com wrote:

I can see the reason for wanting this, I think it's an inherent problem with Mongo really, interesting to know how other libs solve it? (if at all).

Whilst actually I think your code is probably cleaner, would it not be better to just have a migration tool to find references to the previous class in _type fields and update? You'll be constantly carrying around legacy references otherwise.

---

Reply to this email directly or view it on GitHub: https://github.com/coen-hyde/Shanty-Mongo/issues/71#issuecomment-4706157

[tholder]

This certainly isn't a Shanty problem. You're correct that _type means nothing to the driver or mongo but it is a fairly common pattern for other libraries. This doesn't however mean it should be the responsibility of those libraries to deal with a history of client side code refactoring.

Your solution seems fine, but it's a bandaid on a problem caused by the inherent nature of mongo. What happens if you have a User model, refactor it to Person and then later introduce a different User model that represents something different but you still have old _type => 'User' references?

If you're adamant this is necessary, make sure it's passing the tests, stick in a pull request. If @coen-hyde is happy to include it, I'll get it merged.

[coen-hyde]

This will break things. Lets say we have an inheritance structure like this:

      User
          |
 Student     Teacher

If you query Student::all() for example and ::all uses your code then it's going to find all records that have User or Student in their inheritance tree. As such Teacher records will be returned, this is incorrect.

I wouldn't be opposed to a property on the model to turn inheritance off though.

Something like static::$_inheritence = false

[gwagner]

How about an attribute that is controllable. I can see a few instances where I would like $in and $all. We could call it inheritance and have it default to direct with a switch to any or all and then a function that properly appends the right type part to the query before sending.

[gwagner]

Also false to skip inheritance all together like you have listed above

[gwagner]

I committed a revision of code to display the concepts I am describing above https://github.com/gwagner/Shanty-Mongo/commit/1f8594dbc513cc64f8bfbda5b67b4a9a09b4293b

On a side note, i also put a feature in this commit: https://github.com/gwagner/Shanty-Mongo/commit/aaf1e9b1b17f794b366d888e63c8fec575bfdc4a for letting the export function know when field limiting has taken place in a query, and skipping the requirements check before exporting. The developer knows that they limited the fields on query (or else they should be developing) so shanty-mongo shouldn't limit their ability to export if that step has been taken

[coen-hyde]

Hey gwagner,

They are some large commits. Do the tests still pass? Before anything gets into Shanty Mongo it needs to be covered by tests. So it's best to implement features incrementally and test at each step.

Also you'll need to adhere to the project's coding standards as well before it could be merged. We follow the PEAR coding standard, except we use tabs instead of spaces. Personally I wish it used spaces, rather than tabs but that's just the way it is.

[gwagner]

I know it is kindof nit-picky but is there a reason why you follow the PEAR coding standard and not the Zend Framework coding standards for a ZF Mongo Adapter?

I will see what i can do about getting things to pass tests (as i have not even looked into passing tests yet)

[coen-hyde]

When I initially wrote this library, the Zend Framework used the PEAR coding standard, until now i didn't realise they have now defined their own. Ideally it would be good to follow the Zend Framework standards, but that is a separate undertaking.

Until then, any modifications should be consistant with the existing code base.

[gwagner]

The code now passes all tests except for Shanty_Mongo_DocumentTest::testExportRequiredException() For some reason the exception gets thrown when running the class outside of PHPUnit but it wont get thrown when running it in PHPUnit. I did need to modify the tests a little so they worked with MongoDB 2.x (indexes return with v=1 instead of v=0 like 1.x did)

Please review the pull request and let me know what you think