Skip to content

Comments

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

Merged
jeremyharris merged 3 commits intojeremyharris:masterfrom
redscode:master
Dec 12, 2017
Merged

Fixed issue where extending from another entity causes infinite recur…#17
jeremyharris merged 3 commits intojeremyharris:masterfrom
redscode:master

Conversation

@redscode
Copy link

@redscode redscode commented Dec 7, 2017

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

$this->_unsetProperties[] = $prop;
}
return parent::unsetProperty($property);
return Entity::unsetProperty($property);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing blank line before return statement

@codecov-io
Copy link

codecov-io commented Dec 7, 2017

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> (ø) ⬇️

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
Copy link
Owner

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
Copy link
Author

redscode commented Dec 8, 2017

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
Copy link
Owner

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
Copy link
Owner

@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 jeremyharris merged commit 308560f into jeremyharris:master Dec 12, 2017
@jeremyharris
Copy link
Owner

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 :)

protected function &_parentGet($property)
{
return parent::get($property);
return Entity::get($property);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now this really runs? I am baffled.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know. When I asked about it my mind was blown. I'm happy to still be learning :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants