jeremyharris / cakephp-lazyload

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

Fixed issue where extending from another entity causes infinite recur… #17

Closed redscode closed 6 years ago

redscode commented 6 years ago

Fixed issue where extending from another entity causes infinite recursion when lazy loading.

codecov-io commented 6 years ago

Codecov Report

Merging #17 into master will not change coverage. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##             master    #17   +/-   ##
=======================================
  Coverage       100%   100%           
  Complexity       18     18           
=======================================
  Files             1      1           
  Lines            44     44           
=======================================
  Hits             44     44
Impacted Files Coverage Δ Complexity Δ
src/ORM/LazyLoadEntityTrait.php 100% <100%> (ø) 18 <0> (ø) :arrow_down:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 6a8c8a2...40ffe79. Read the comment docs.

jeremyharris commented 6 years ago

Forgive my ignorance, but how the heck does this even work? Entity doesn't have a static get and even if it did it wouldn't have state. Am I missing something?

I think self might be appropriate here, would that fix your issue? Can you supply a minimal test case version of the entity you're using?

redscode commented 6 years ago

Well, Entity or EntityTrait doesn't have a static get either so anything that extends those wouldn't have it unless it's added manually. I was following the logic that the LazyLoadTrait would be applied to a class that extends Entity. So by parent:: we are really accessing Entity::

I tried using self:: and static:: and encountered the same problem. Only Entity:: remedies the problem.

To replicate you need to extend from an existing entity that uses the LazyLoadTrait (both classes have to use the trait). You also need to create a Table class for the new entity that is set to the original models table. Then when you have the new model and you try to lazy load anything you'll hit the bug.

lazy_test.zip `

jeremyharris commented 6 years ago

My confusion was how Entity:: works at all; it shouldn't unless I'm misunderstanding something about PHP. Perhaps my tests have a huge hole!

At any rate, I'll test it out, thanks for the zip file!

jeremyharris commented 6 years ago

@redscode while I look into this, you can fix your issue by only including the trait in the base class. The child class will inherit the lazy loading abilities (I probably should update the documentation to better explain this).

jeremyharris commented 6 years ago

Okay, I learned something new today!

I was trying to understand why Entity:: worked here, and learned from some people much smarter than I (you, markstory and admad). Apparently, if you are calling a parent method of the same name, you can target a particular ancestor's method by using it in a static-like context.

So yay, thanks for reporting this bug and for teaching me something new :)