-
Notifications
You must be signed in to change notification settings - Fork 48
Typemap refactor #21
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
Typemap refactor #21
Conversation
public function testGetSingleTableTypesOfRoot() { | ||
$types = ['motorvehicle', 'car', 'truck', 'bike']; | ||
$types = ['motorvehicle', 'bike', 'car', 'taxi', 'truck']; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
on the surface its kinda terrifying that this expectation changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me it's correct as a taxi is a type of Vehicle. The only other change is the ordering but that's only down to how the type map is built.
I would expect to see a new test case the prove the problem that this change is expected to fix is truly fixed |
Test case added which ensures the correct model types are instantiated when loading relationships. |
Just to note on this, the failing tests on HHVM appear to be due to a bug in HHVM: facebook/hhvm#7193 The bug causes the code to get into an infinite loop and crash. I can work around it by checking |
@bostanio Any interest in this or anything you would like clarifying? |
@alexwhitman I am interested in this... +1 |
…ving the issue reported in #20 isn't a bug.
I added the test here 6302274 and it passed without these changes. |
Refactors how the type map is built to fix #20. The type names are now defined as keys on the subclass array rather than in the model.
All of the tests pass with some slightly refactored to handle the change.
This is a breaking change.