jeremyharris / cakephp-lazyload

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

By-reference array functions don't work with LazyLoadableEntity entities #18

Closed themrwilliams closed 6 years ago

themrwilliams commented 6 years ago

Example here: https://github.com/themrwilliams/cakephp-lazyload/commit/185aa5a4befb3b4a3e016ea79d5bb9a6894d4c2d

Both tests are the same scenario. The first one, with the trait disabled, passes. The second one fails.

I plan to dig into it more when I have time, but opening this for now in case someone has already run into a similar issue before.

EDIT: This does seem to apply to any of the by-reference array functions (e.g. array_pop, etc.).

josephshanak commented 6 years ago

_lazyLoad doesn't return a reference.

This seems to fix the test case (at the expense of calling _parentGet twice):

diff --git a/src/ORM/LazyLoadEntityTrait.php b/src/ORM/LazyLoadEntityTrait.php
index 1b84e7a..d51afa2 100644
--- a/src/ORM/LazyLoadEntityTrait.php
+++ b/src/ORM/LazyLoadEntityTrait.php
@@ -33,12 +33,11 @@ trait LazyLoadEntityTrait
     public function &get($property)
     {
         $get = $this->_parentGet($property);
-
         if ($get === null) {
             $get = $this->_lazyLoad($property);
         }

-        return $get;
+        return $this->_parentGet($property);
     }

     /**

We could probably change things around a bit so _lazyLoad returns a reference to $this->_properties[$property] without having to call _parentGet unnecessarily.

themrwilliams commented 6 years ago

It looks like the double call can be avoided by just assigning $get by reference:

diff --git a/src/ORM/LazyLoadEntityTrait.php b/src/ORM/LazyLoadEntityTrait.php
index 1b84e7a..e3e1d2f 100644
--- a/src/ORM/LazyLoadEntityTrait.php
+++ b/src/ORM/LazyLoadEntityTrait.php
@@ -32,7 +32,7 @@ trait LazyLoadEntityTrait
      */
     public function &get($property)
     {
-        $get = $this->_parentGet($property);
+        $get = &$this->_parentGet($property);

         if ($get === null) {
             $get = $this->_lazyLoad($property);

All the existing tests pass, at least.

We probably don't want to go straight into $this->_properties b/c we need the parent get to run the accessors and whatnot.

Other odd things... LazyLoadEntityTrait::_parentGet calls Entity::get, which isn't a static method. Seems to work fine though.

jeremyharris commented 6 years ago

For info on why Entity::get works even though there's no static method, check out https://github.com/jeremyharris/cakephp-lazyload/pull/17#issuecomment-351170070. It was a new syntax to me, too!