Skip to content

Fix #16411, added ORM SessionCache AKA FirstLevelCache - #16450

Open
rudiservo wants to merge 1 commit into
phalcon:5.0.xfrom
rudiservo:i16411
Open

Fix #16411, added ORM SessionCache AKA FirstLevelCache#16450
rudiservo wants to merge 1 commit into
phalcon:5.0.xfrom
rudiservo:i16411

Conversation

@rudiservo

Copy link
Copy Markdown
Contributor

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 have updated the relevant CHANGELOG
  • I have created a PR for the documentation about this change

Small description of change:

Thanks

@rudiservo
rudiservo force-pushed the i16411 branch 2 times, most recently from 7e21423 to ddbcbe2 Compare October 2, 2023 12:18
@rudiservo

Copy link
Copy Markdown
Contributor Author

SqLite tests fail, the tests are fixed in one of the PR I previously made, I will rebase this once they are approved.
feel free to check and comment.

@niden

niden commented Apr 24, 2026

Copy link
Copy Markdown
Member

@rudiservo I had some time (finally) to check this one.

I see the benefit and am not opposed to it but there are some concerns I have in particular with the __destruct part and orphan references.

We can certainly add checks for objects that are null (modelsManager etc.) and that is fine but I am worried about the internal dependencies of objects and the references that they hold to other objects. Therefore the __destruct is the weak point here (and we don't have tests for that). If for whatever reason, the DI container is destructed before the model, then we will be accessing a dead reference which will cause a segfault. Static variables have always been fragile so that is something to consider.

Finally, do we really need a reference to the modelsManager? Can't we use the cache to store all that and only keep the key?

@rudiservo

Copy link
Copy Markdown
Contributor Author

Hey, the __destruct will be called by the php GC, meaning that the worst case scenario is a GC bug and there is no longer a single source of truth, that is what we have now with the current system, this is only to remove keys that no longer has any stable reference.

So the philosophy of a first level cache system is a single source of truth, meaning that anywhere on the current thread the Model called is always the same.

I think we could pass a cache, in fact, if we are using something like Swoole or Kraken we should have a new cache for each thread, because every thread is independent on its working data.

I'll check it out.

@rudiservo

Copy link
Copy Markdown
Contributor Author

still checking this btw.

@rudiservo

Copy link
Copy Markdown
Contributor Author

OK so I get what you mean by models manager, going to remove.

To get the caching situation cleared
SessionCache and cache are two different things in the resultset and in ORM theory

Thread cache/session cache/First Level cache, is only for that request and it's use in PHP with the WeakCache that I added before, so WeakCache will work with the current thread and only know object references via WeakReference, I left it with the Abstract adapter so anyone can make a different cache if they need to for their use case.

The other one is considered the second level cache, slower and longer lived, that is for redis, memcached or other.

I dont like using __destruct, I don't know what to use to clear the keys in the WeakCache so it wont grow indefinitely, a problem that only affects long lived requests or background tasks.

The only solution I have if a make a Check() function in the weakCache to go over all keys find nulls and deference them so a developer call it if he runs things longer then a quick request, and the rest just let the cache clear out by itself.

I will make these changes and update the PR.

@rudiservo
rudiservo force-pushed the i16411 branch 11 times, most recently from 29a42dc to be77ab0 Compare August 11, 2026 16:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants