Fixed saving a model after fetching its related records

[zsilbi]

Hello!

• Type: bug fix
• Link to issue: [BUG] Updating a model fails after fetching its related records #13964

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

Small description of change:

I've added some tests to check if this issue exists at all.
The tests failed, so added a fix in a previous PR (#13970) but ran into a conflict so I made a new PR.

The problem is, that the related models are stored in the _related storage inside Model after accessing via __get(). It is not stored when accessing related records via __call().

When the model is saved, _postSaveRelatedRecords is being called and it iterates through _related and tries to save the previously fetched related records.

For now, I just removed adding these records to the _related as it's not mandatory when you are only reading them.

Thanks

[codecov]

Codecov Report

Merging #13985 into 4.0.x will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           4.0.x   #13985      +/-   ##
=========================================
- Coverage   67.9%    67.9%   -0.01%     
=========================================
  Files        469      469              
  Lines      94291    94290       -1     
=========================================
- Hits       64032    64031       -1     
  Misses     30259    30259

Impacted Files	Coverage Δ
ext/phalcon/mvc/model.zep.c	66.6% <0%> (-0.01%)	⬇️

---

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 143532d...6d0ef9d. Read the comment docs.

[sergeyklay]

I will keep this open for a some time to allow other contributors a chance to speak up, and to take a fresh look at it later. But at the moment it seems good to me.

/cc @phalcon/framework-team

[niden]

@zsilbi can you update the changelog when you get a chance please and rebase.

Thanks!

[virgofx]

We need another test as it looks like this removed functionality. I'm not sure with regards to the run-time exception or what was generating ... but this is likely not the right fix as this will break caching.

Need to query a related record multiple times and ensure the database is only hit once. In this case, it will be re-queried every time when you remove that line.

[zsilbi]

We need another test as it looks like this removed functionality. I'm not sure with regards to the run-time exception or what was generating ... but this is likely not the right fix as this will break caching.

Need to query a related record multiple times and ensure the database is only hit once. In this case, it will be re-queried every time when you remove that line.

1. In Model::__get() relations are obtained via Model/Manager::getRelationByAlias(). Manager caches them properly.
2. Queried related objects are stored and cached in properties of the Model. This functionality is not affected by this PR.

[niden]

Thank you @zsilbi

[virgofx]

Well now isRelationshipLoaded won't return correctly. If you're going to refactor the caching of related items ... it needs to be done everywhere. If we somehow double cached (manager + model) that needs to be more conveyed and fixed in all locations. Commenting out one line breaks other areas. As I mentioned ... this still will re-query ... so it's broken as you haven't tested for that anywhere. Related is also protected and end-users have extended that for various ways to inject related data not just from magic methods as well.

$model->getRelatedRobotParts();  // [ manager cache = false, model = false ]
$model->getRelatedRobotParts();  // [ manager cache = true, model = false ] query [ bad]
$model->getRelatedRobotParts();  // [ manager cache = true, model = false ] query [ bad]

If a runtime exception is being thrown somewhere ... the fix is to not UNCOMMENT a line .. it's to fix the issue where the exception is being thrown and explain it properly with tests.

This needs to not be merged @niden. Revert please :) until a more thorough analysis can be done with proper tests showing this won't all of a sudden overwhelm production stacks that use getters for related items all over not expecting the database to be hit everywhere.

[zsilbi]

Well now isRelationshipLoaded won't return correctly. If you're going to refactor the caching of related items ... it needs to be done everywhere. If we somehow double cached (manager + model) that needs to be more conveyed and fixed in all locations. Commenting out one line breaks other areas. As I mentioned ... this still will re-query ... so it's broken as you haven't tested for that anywhere.

This needs to not be merged @niden.

If a runtime exception is being thrown somewhere ... the fix is to not UNCOMMENT a line .. it's to fix the issue where the exception is being thrown and explain it properly with tests.

@virgofx it's a different issue then because if you query the items via Model::__call() (for example $robot->getRobotParts()) the relation is never added to the Model::related[] anyway.

If you investigate further you will notice that this related storage is only used before and after updating a models related records. Only if this storage is empty Model::_preSaveRelatedRecords() and Model::_postSaveRelatedRecords() will not run during an update.

[sergeyklay]

Just reverted this PR from 4.0.x branch

[sergeyklay]

@zsilbi Feel free to discuss the issue here and re-send a new one, again if the need arises

[virgofx]

@zsilbi I'm not disagreeing that the original implementation is poor. If we're going to remove the related ... it should be removed everywhere and we should have tests to account for what I described above. It's still being used when setting related items and checking if it's loaded. So if we're going to remove the related attribute for handling that .... all 3 things need to be taking care of:

1 - getting related
2 - setting related
3 - checking if relationship is loaded (what a silly method) ... i do believe that isset may call into __call/__get which is why we originally duplicated them into the related bag. If so ... we need to keep it in point 1 which your PR removed.
4 - tests - all 3 above +
please add:
i] - multiple repeat getting related -- instantiate a new SQL logger of the queries to show only the first one is set.

5 - if we do this .. .it's also breaking (fine for 4.0) ... just need to make aware people that extend the Model class with custom relations/logic (I do this myself) so the comments need to be updated to reflect this more thoroughly in the 4.0 upgrade log as well.

[zsilbi]

Ok guys, I've done some research to get the complete picture about the purpose of Model::related[]

In #12772 @Surt mentoined that the former Model::_related[] (now related[]) storage is only used for belongs-to relations. Maybe that's why it must've failed when it was populated with other type of relations.

I am not sure if @CameronHall had been aware of this in #13616 when he'd added that line that I commented here. When Model::isRelationshipLoaded() was implemented, it only returned true when the related records were previously accessed as a property ($robot->robotParts). It passed the tests that time because they were only written to cover this case.

Also, this related storage problem needs to be redone together with issue #13531, as storing objects only in properties as a cache and using isset() will make the reusable option uneffective.

Please advise.

[virgofx]

Like I said ... the related records is very finicky and any changes will either be super minor ... or a refactor and need to cover the cases I've mentioned above. We have to be very careful because I have made changes in extending models and small changes like this cause issues where repeat queries kill production servers .... from an end-users application logic ... they may want to use $myModel->getFancyRelated()->getValue() in numerous places throughout code.

[CameronHall]

I agree isRelationshipLoaded is a silly method. I only added it because I was cleaning up the PR backlog. In hindsight, we should only be able to rely upon __isset to handle this.