Identifier test for issue #29

[lordphnx]

First time making a PR, so apologies if I am doing something nooblike, would love to hear it :)

[jeremyharris]

Thanks for the PR!

It looks like your editor touched just about every file which triggered the CS rules for everything, hence the noise. In the future when doing PRs make sure to only commit the files and changes relevant to the issue - this makes it easier for maintainers to review the code.

That said, I see the test case you added and I can rebase this branch and clean things up.

I really appreciate you putting together the test case! I'll try to get this resolved for you :)

[lordphnx]

I probably should have included this in the PR, but this changes fixes it for me.

- if ($association === null || $this->get($association->getBindingKey()) === null) {
+ if ($association === null || $this->get($association->getForeignKey()) === null) {
   return null;
}

[jeremyharris]

Hey @lordphnx, sorry for taking so long. I cleaned up and pushed to your branch. Please take a look to make sure the test case matches your expectations.

The test passes, so I'm not sure if there's a different problem or if I misunderstood the problem. FWIW I still think the link in question is incorrect, I just want to make sure to have a complete understanding of what's failing now before fixing it.

[lordphnx]

Hey @jeremyharris thanks for getting back to me. Just to confirm it's `testAliasedAssociationsWithSameClass?

I get a checkmark too, I'll spend some time in the next few days to see if I can break it 😉

[jeremyharris]

Yep that's the one. Feel free to tweak to reproduce the error!

[lordphnx]

So this here mainly for my future reference, as my head is currently fried:

• in my identifier branch, once we dip into LazyLoadEntityTrait::131 for the Author association $association->getbindingKey() returns user_id instead of author_id.
• In Jeremy's branch, it neatly returns author_id, which we expect.
• So probably it's somewhere in the ORM definition of my test?

[lordphnx]

@jeremyharris I've done some minimally invasive edits to "break" it. Although, my head is fried, so my test might just be broken.

I was suspicious of author_id and "Authors", because cake might be able to derive author_id from "Authors" So I changed Editor and Author to belongsTo("Users").

Although my reasoning there doesn't really make sense, because you'd still expect it to break for Editor

[jeremyharris]

By the way, ignore the failing check on github for now. It's not running tests as noted in #28 so you'll have to run them locally.

I think I see where you're going with your changes though. You're changing the primary key of the parent table, perhaps the issue you're running into is when the primary key on the associated tables is changed - so try changing the PK on Authors instead.

The bugged line is it trying to fetch a binding key (e.g. authors.id) on the parent model (articles) - this obviously exists in conventional situations where both articles and authors have an id field, so it finds the articles.id. However if the PK on authors is author_id it'll look for articles.author_id. This exists as well in this case, so try changing the Authors PK to something like author_pk which definitely doesn't exist on articles.

[lordphnx]

Hey Jeremy, just to check, is the ball back in my court for this?

[jeremyharris]

If the comment above makes sense then try and re-adjust your test to see if you can still replicate. If not I'll take a swing - I think I understand your test case well enough.

[jeremyharris]

Hi @lordphnx. So sorry for the delay on this, I finally set aside time to work on the issue. I came up with this:

https://github.com/jeremyharris/cakephp-lazyload/pull/31

Changing to an unconventional PK definitely breaks the behavior. If you have some time please take a look at the PR above to see if it fixes your issue.

[lordphnx]

Hey @jeremyharris, sorry for not taking this up myself, life got in the way (you know how it goes). Looking at the changes you made to your test and fixture, this definately covers the edge-case I was encountering.

With respect to the fix you are making in the trait itself:

• I see where you're going with this, trying to see if the property is physicall there, and if not handling that case

• The fix I had in mind:

• if ($association === null || $this->get($association->getBindingKey()) === null) {

• if ($association === null || $this->get($association->getForeignKey()) === null) {

• My $this->get($association->getForeignKey()) === null) achieves approximately the same as your

• !isset($this->_fields[$association->getForeignKey()])

But perhaps there is a case that my fix misses that your fix covers, my understanding of loadInto doesn't go that deep 😅

[jeremyharris]

No problem, totally understand.

The fixes are basically the same except that get() will return null even if the property isn't on the entity and it'll trigger a lazy load attempt. I can't remember but I'm pretty sure using get here broke some tests which is why I ended up using isset.

The new PR also checks if the association is BelongsTo since other associations will definitely not have a key on the primary table.

[jeremyharris]

Closed in favor of #31