jeremyharris / cakephp-lazyload

A lazy loader for CakePHP entities.
MIT License
61 stars 10 forks source link

4.0.2 not backwards compatible with 4.0.1 #29

Closed lordphnx closed 2 years ago

lordphnx commented 2 years ago

I just updated to 4.0.2 and several of my unit tests failed. Reverting to 4.0.1 fixed the issue. I've so far managed to track the failure down to resolving a belongsTo association

Context

Possible solution

Probably this is me not grasping something, but in the check: $this->get($association->getBindingKey()) === null (LazyLoadEntityTrait line 131) why is $association->getBindingKey() (i.e. user_id) gotten on the object we are lazy loading into (an instance of issue). A check of $this->get($association->getForeignKey()) === null seems to make more sense to me?

Apologies for this very "vague" issue, but felt I had to post it anyway. I'm not very familiar with these types of submissions, so feel free to give my tips/pointers/suggestions for improvements.

jeremyharris commented 2 years ago

Thanks for the detailed issue. I'll try to put together a fix asap. I apologize for the non-BC release!

If I cannot fix it while supporting #27 relatively quickly I'll just push a release reverting the change.

lordphnx commented 2 years ago

No apologies necessary! Have been happily using this plugin for years now! Glad to be able to "contribute" (albeit minor)

jeremyharris commented 2 years ago

I can't seem to create a test case that fails. I added another field/association to the ArticlesFixture, editor_id which is also an Author (like author_id), and things seem to work just fine.

I think the fix was still incorrect, it shouldn't be checking for an association's binding key on the current table, it should just check the primary key. I'd like to get a failing test case for this issue in before to make sure that it's covered as well.

lordphnx commented 2 years ago

I'm currently quite swamped. I can attempt to port my code into something I'm allowed to publish, but it will be monday before I find time.

jeremyharris commented 2 years ago

I completely understand! If it makes things easier you can use/modify the fixtures in the repo.

lordphnx commented 2 years ago

So a bit later than monday (you know how it goes). I have a test that works on 4.0.1 but fails on 4.0.2.

So my next (newbie) question is: how do I get it to you? (as I'm not allowed to push to a new branch)

jeremyharris commented 2 years ago

You can either fork and create a PR from your fork, or, drop it here and I'll put it in for you. The CI server is broken on this repo at the moment so I'll have to run the tests locally and come up with a fix regardless.