Skip to content
This repository has been archived by the owner on May 26, 2023. It is now read-only.

Relationships don't construct cache keys correctly #109

Closed
harto opened this issue May 5, 2014 · 10 comments
Closed

Relationships don't construct cache keys correctly #109

harto opened this issue May 5, 2014 · 10 comments
Labels
Milestone

Comments

@harto
Copy link
Collaborator

harto commented May 5, 2014

The cache key is incorrectly constructed in BelongsTo::get() and HasOne::get().

In BelongsTo::get(), the cache key is constructed something like this:

$schema = \Pheasant::instance()->schema($this->class);
$cacheKey = $schema->hash($object, array($this->foreign));

Using an example from the test suite:

  • $object is an instance of Hero
  • $this->class is the class of the related object, i.e. SecretIdentity
  • $this->foreign is the name of SecretIdentity's primary key column, e.g. id.

For a Hero with id = 1 and identityid = 2, the above code will generate a cache key of SecretIdentity[id=1] instead of SecretIdentity[id=2].

I believe the test suite is only passing because the sequences table is wiped each time, and so each Hero->id always matches SecretIdentity->id.

@harto
Copy link
Collaborator Author

harto commented May 5, 2014

Ping @lox -- I'm not really sure how to approach this one.

@lox
Copy link
Owner

lox commented May 5, 2014

Ok cool, I'll see what I can do. Any chance you could commit a failing testcase to a branch?

@bjornpost
Copy link
Collaborator

@lox: I guess the testcase in #105 is still up-to-date. Let me know if I can test anything (or help with something else).

harto added a commit that referenced this issue May 8, 2014
The test that now fails (`IncludesTest::testIncludesHitsCache()`) was
only passing because there was a 1-to-1 mapping between the IDs of
related entities. I.e. Hero with ID=1 corresponds to SecretIdentity with
ID=1, etc.

This forces SecretIdentity IDs to start at 100, which exposes the bug
discussed in #109.
@harto
Copy link
Collaborator Author

harto commented May 8, 2014

This is a slightly simpler failing test: 1d0632d

@bjornpost
Copy link
Collaborator

Any progression on this? Maybe we can start preparing a 1.4 release which introduces include(), Scopes and $allowEmpty for BelongsTo. I've also been working on and off improving the docs, making sure all concepts are explained again.

@lox lox added this to the 1.4.0 milestone Sep 4, 2014
@bjornpost
Copy link
Collaborator

@lox is there any progress on this? Is there anything I can do to help speed things up?

@bjornpost
Copy link
Collaborator

Cool! Would you mind testing this against the test in 1d0632d?

@lox
Copy link
Owner

lox commented Mar 20, 2015

If this fixes the issues, I'll merge and release promptly.

@Jud
Copy link
Collaborator

Jud commented Mar 20, 2015

Fixed in #126

@lox lox closed this as completed in ab6c67d Mar 21, 2015
@bjornpost bjornpost modified the milestones: 1.4.0, 2.0.0 May 7, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

4 participants