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.

Sign up for GitHub

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

Jump to bottom

[WIP] Refactored Model #12328

Merged
merged 31 commits into from
Oct 21, 2016
Merged

[WIP] Refactored Model #12328

merged 31 commits into from
Oct 21, 2016

Conversation

SidRoberts
Copy link
Contributor

Don't merge this just yet - there'll be a few more commits in the next few days.

@sergeyklay
Copy link
Contributor

@SidRoberts It is changes for 3.0.x or for 3.1.x ?

@SidRoberts
Copy link
Contributor Author

3.0.x - I'm just simplifying the code in the existing methods for now.

@sergeyklay sergeyklay added the enhancement Enhancement to the framework label Oct 16, 2016
@sergeyklay sergeyklay added this to the 3.0.2 milestone Oct 16, 2016
@sergeyklay
Copy link
Contributor

sergeyklay commented Oct 17, 2016 •
edited
Loading

@SidRoberts It seems all ok with testы. You can continue your work
Refs:

@SidRoberts
Refactored Model::validate().
2914874
@SidRoberts
Refactored Model::hasChanged().
f14bc24
@SidRoberts
Refactored Model::_checkForeignKeys*().
d3437f6
@SidRoberts
Refactored Model::skipAttributes().
5a32915
@SidRoberts
Refactored ModelManager::setConnectionService().
c618e0c
@SidRoberts
Refactored ModelManager::get*Records().
e74ca46
@SidRoberts
Refactored ModelManager::get*().
590d9ec
@SidRoberts
Removed unused variables.
e4bd0ce
@SidRoberts
Refactored ModelManager::getRelationsBetween().
d679236
@SidRoberts
Refactored ModelManager::getModelSource().
26de8d2
@SidRoberts
Refactored ModelManager::notifyEvent().
1a81368
@SidRoberts
Refactored ModelManager::getModelSchema().
b1bb3f2
@SidRoberts
Refactored ModelManager::missingMethod().
2368a9a
@SidRoberts
Refactored ModelManager::_getConnectionService().
bf3f8bd
@SidRoberts
Refactored Model::setSnapshotData().
fe17fac
@SidRoberts
Refactored ModelRelation::isForeignKey().
cda59fb
@SidRoberts
Refactored ModelMetadata::(read|write)MetaDataIndex().
1ff620a
@SidRoberts
Removed call_user_func_array calls from Model Transaction.
6692143
@SidRoberts SidRoberts closed this Oct 18, 2016
@SidRoberts SidRoberts reopened this Oct 18, 2016
sergeyklay
sergeyklay suggested changes Oct 18, 2016
View reviewed changes
phalcon/mvc/model/relation.zep
}
}
return false;
return isset this->_options["foreignKey"];
Copy link
Contributor

Choose a reason for hiding this comment

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

protected _options;

not array

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it isn't, isset will return false anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

But won't php complain about this ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It won't because it's reasonable to assume that the variable isn't set anyway - that's why we're using isset(). Technically it's a language construct, not a function, so different rules apply.

Copy link
Contributor

@Jurigag Jurigag Oct 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Yea i tested and it's fine in php at least.

@SidRoberts
Copy link
Contributor Author

@sergeyklay Assuming this passes in Travis, I think it's ready to merge.

@Jurigag
Copy link
Contributor

Jurigag commented Oct 19, 2016

Squash please :)

@SidRoberts
Copy link
Contributor Author

I'm not on my dev machine and Serghei can rebase it when merging.

sergeyklay
sergeyklay reviewed Oct 21, 2016
View reviewed changes
phalcon/mvc/model/transaction.zep
@@ -149,7 +149,7 @@ class Transaction implements TransactionInterface

let manager = this->_manager;
if typeof manager == "object" {
call_user_func_array([manager, "notifyRollback"], [this]);
manager->notifyRollback(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

@sergeyklay sergeyklay merged commit 29e8771 into phalcon:3.0.x Oct 21, 2016
@sergeyklay
Copy link
Contributor

Thanks

@SidRoberts SidRoberts deleted the model-refactor branch October 21, 2016 11:35
@sergeyklay
Copy link
Contributor

@SidRoberts Now builds for 3.0.x are broken. https://travis-ci.org/phalcon/incubator/builds/169510034

Could you please take a look

@SidRoberts SidRoberts mentioned this pull request Oct 22, 2016
Merged
@SidRoberts
Copy link
Contributor Author

Fixed in phalcon/incubator#680.

@SidRoberts SidRoberts mentioned this pull request Oct 22, 2016
Merged
@Jurigag
Copy link
Contributor

Jurigag commented Oct 24, 2016 

Warning: Variable "metaData" declared but not used in Phalcon\Mvc\Model\MetaData::writeMetaDataIndex in /root/cphalcon/phalcon/mvc/model/metadata.zep on 305 [unused-variable]

          var metaData, source, schema, key;

@SidRoberts

@sergeyklay sergeyklay mentioned this pull request Oct 24, 2016
Merged
@sergeyklay
Copy link
Contributor

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergeyklay sergeyklay sergeyklay approved these changes

@Jurigag Jurigag Jurigag left review comments

Assignees

@SidRoberts SidRoberts

Labels
enhancement Enhancement to the framework
Projects
None yet
Milestone
3.0.2
Development

Successfully merging this pull request may close these issues.
3 participants
@SidRoberts @sergeyklay @Jurigag