fix query_cache behavior compatibility with Criteria and ModelCriteria #484
Conversation
|
what is the issue you are fixing? can you add unit tests for it? |
|
There is no open issue, I just found this bugs when I want to use this behavior. I search for existing unit tests for this behavior but they not exists. How to test it ? I create a schema like timestampable behavior or I define the schema in the test like I18n ? |
|
Yeah, I guess it is tricky to test. However, as we might be able to inject our own backend, we might be able to mock a backend to test the behavior. WDYT? |
|
because the api you changed requires code-generation you could do it like define a schema, build it using QuickBuilder and run your tests...? I am not familiar with this behavior therefore I could be wrong.. But I am wondering that you need to make those methods public (and therefore creating new API surface). How did it work before (e.g. in Propel1)? |
|
I create a first test. I don't think we have to use a custom backend, array backend is enought. What do you think ? I still like that ? lunika@864cd01 |
|
Thats a good start. But the reason why @willdurand said we need a custom backend is, that you can't verify at the moment, that you query was answered by the cache and not by the DB. Maybe you could create a mock for such a backend and do the assertion then see |
|
I add some more tests. @staabm the query is always answered by the server. The QueryCache behavior only cache the sql code. |
| @@ -0,0 +1,13 @@ | |||
| <?xml version="1.0" encoding="ISO-8859-1" standalone="no"?> | |||
| <database name="bookstore-behavior" defaultIdMethod="native" package="behavior.timestampable" namespace="Propel\Tests\Bookstore\Behavior"> | |||
There was a problem hiding this comment.
you can remove the package attribute I guess.
|
@lunika please fix the CS stuff first, then rebase your PR and I'll merge it :) |
|
I fix cs and remove useless package attribute. When I rebase, git says that I'm already up to date so I guess it's ok ? |
|
Ah right. I meant "squash" your commits ;-) |
with ModelCriteria class return DataFetcherInterface instance for doCount and doSelect methods in query_cache behavior
|
I think I have to create a new PR in a new branch and delete this one. When my commits are squashed I can't push the new one because old commits are already push. What do you think ? |
|
You have to force the push. try |
|
Great ! Thank you very much |
fix query_cache behavior compatibility with Criteria and ModelCriteria
|
thanks! test suite is broken but not your fault. |
|
Thanks everybody for your help |
No description provided.