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

Mvc\Model::__get() / __set() and related storage #14035

Merged
merged 53 commits into from
May 11, 2019

Conversation

zsilbi
Copy link
Member

@zsilbi zsilbi commented May 2, 2019

Hello!

In raising this pull request, I confirm the following:

  • I have read and understood the Contributing Guidelines
  • I have checked that another pull request for this purpose does not exist
  • I wrote some tests for this PR
  • I updated the CHANGELOG

Added new related storages
Modified __set(), __get(), getRelated() to use these related storages
Modified save() to use these related storages with transaction handling
Modified _getRelatedRecords() to use getRelated() when possible
Reusable relations are cached and returned from the Model directly
Non-reusable relations are not cached
Fixed __set() to handle arrays correctly

Added tests for Mvc\Model::save()
Added tests for Mvc\Model::__set()
Added tests for Mvc\Model::writeAttribute()
More tests needed...

Thanks,
zsilbi

zsilbi and others added 10 commits April 16, 2019 16:34
Added tests for Mvc\Model::save()
Added tests for Mvc\Model::__set()
Added tests for Mvc\Model::writeAttribute()
Added new related storages
Modified __set(), __get(), getRelated() to use these related storages
Modified save() to use these related storages with transaction handling
Modified _getRelatedRecords() to use getRelated() when possible
Reusable relations now cached and returned from the Model directly
Non-reusable relation are not cached
Fixed __set() to handle arrays correctly
@codecov
Copy link

codecov bot commented May 3, 2019

Codecov Report

Merging #14035 into 4.0.x will decrease coverage by 1.71%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##            4.0.x   #14035      +/-   ##
==========================================
- Coverage   70.55%   68.84%   -1.72%     
==========================================
  Files         479      470       -9     
  Lines       93433    95198    +1765     
==========================================
- Hits        65924    65539     -385     
- Misses      27509    29659    +2150
Impacted Files Coverage Δ
ext/phalcon/mvc/model/metadata/apcu.zep.c 8.33% <0%> (-87.23%) ⬇️
ext/phalcon/filter/sanitize/specialfull.zep.c 23.07% <0%> (-76.93%) ⬇️
ext/phalcon/1__closure.zep.c 25% <0%> (-56.82%) ⬇️
ext/phalcon/12__closure.zep.c 25% <0%> (-56.82%) ⬇️
ext/phalcon/11__closure.zep.c 25% <0%> (-56.82%) ⬇️
ext/phalcon/13__closure.zep.c 25% <0%> (-56.82%) ⬇️
ext/phalcon/8__closure.zep.c 37.5% <0%> (-44.32%) ⬇️
ext/phalcon/3__closure.zep.c 37.5% <0%> (-44.32%) ⬇️
ext/phalcon/15__closure.zep.c 37.5% <0%> (-44.32%) ⬇️
ext/phalcon/4__closure.zep.c 37.5% <0%> (-44.32%) ⬇️
... and 122 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55a7139...d90459b. Read the comment docs.

@sergeyklay
Copy link
Contributor

/cc @phalcon/framework-team

ruudboon and others added 14 commits May 5, 2019 12:22
Added missing comments
* Added variable types.

* Whitespace.

* Improved logic.
…n#14026)

* Fixeing Config\Adapter\Ini notices when casting arrays

* Fixed Phalcon\Config\Adapter\Ini() to handle arrays correctly

* Update tests/_data/assets/config/config-with-constants.ini

Co-Authored-By: zsilbi <[email protected]>

* Fixeing Config\Adapter\Ini notices when casting arrays

* Fixed Phalcon\Config\Adapter\Ini() to handle arrays correctly
Added tests for Mvc\Model::save()
Added tests for Mvc\Model::__set()
Added tests for Mvc\Model::writeAttribute()
@zsilbi
Copy link
Member Author

zsilbi commented May 7, 2019

@CameronHall I checked SplObjectStorage but I dont't think it would be beneficial to use here.
The related records are indexed by their aliases, not by their content (hash).
On the other hand, I think in Manager, if you have to store any data for a given object, it could work.

@zsilbi zsilbi changed the title [WIP] Mvc\Model::__get() / __set() and related storage Mvc\Model::__get() / __set() and related storage May 7, 2019
@niden
Copy link
Member

niden commented May 10, 2019

@zsilbi can you please rebase this?

This looks great. Awesome work!

@zsilbi zsilbi force-pushed the model-relation-storage-with-tests branch from 35c5299 to d90459b Compare May 11, 2019 14:16
@niden niden merged commit d90459b into phalcon:4.0.x May 11, 2019
@niden
Copy link
Member

niden commented May 11, 2019

Thank you @zsilbi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug A bug report enhancement Enhancement to the framework status: low Low status: medium Medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants