spotorm / spot2

Spot v2.x DataMapper built on top of Doctrine's Database Abstraction Layer
http://phpdatamapper.com
BSD 3-Clause "New" or "Revised" License
601 stars 101 forks source link

Custom getter should be able to access the field directly #294

Closed sylbru closed 4 years ago

sylbru commented 4 years ago

When I define a getter like this:

public function getSite()
{
    $site = $this->Site;

    if (!preg_match("~^https?://.+$~", $site)) {
        return "http://" . $site;
    } else {
        return $site;
    }
}

And I try to access the Site property like this:

$site = $object->Site;

I get an exception inside the getter saying the Site property is undefined, even though it worked fine the first time ($object->Site), thanks to the __get magic method. Why is that?


There is also, I think, a mistake in the __get magic method, as it sets $this->_inGetter[$field] = true while checking !in_array($this->_inGetter, $field). $field is put in the keys of the array, so it would not be found by the in_array function. I guess a !empty or array_key_exists would work.

FlipEverything commented 4 years ago

Could you please rename Site to site and test it again?

I think that is required for CamelCase: https://github.com/spotorm/spot2/blob/4867f7c1d91baf5772288d96b89d8a67bd4fa5be/lib/Entity.php#L442

sylbru commented 4 years ago

I changed it to site with "column" => "Site", and changed both occurrences of ->Site to ->site, it doesn't seem to make a difference.

FlipEverything commented 4 years ago

You have to change it via the Entity definition ("site" => [...]). The column key is only responsible for the sql naming.

sylbru commented 4 years ago

That's what I meant, I can see now that what I wrote is really not clear:

public static function fields()
{
    return [
    ...
        "site" => ["column" => "Site", "type" => "string", "length" => 150],
    ...
    ];
}
FlipEverything commented 4 years ago

You have to modify your getter to make it work:

public function getSite()
{
    $site = $this->get('site');

    if (!preg_match("~^https?://.+$~", $site)) {
        return "http://" . $site;
    } else {
        return $site;
    }
}

Test code:

$appMapper = $app->mapper('Application');

foreach ($appMapper->all() as $object) {
    var_dump($object->site);
}

Output:

/var/git/Entity/Application.php:18:
string(16) "https://test.com"
/var/git/test.php:8:
string(16) "https://test.com"
/var/git/Entity/Application.php:18:
string(15) "http://test.com"
/var/git/test.php:8:
string(15) "http://test.com"
/var/git/Entity/Application.php:18:
string(8) "test.com"
/var/git/test.php:8:
string(15) "http://test.com"
sylbru commented 4 years ago

This is precisely what I don't understand: why doesn't ->site work from inside the getter? It doesn't even seem to trigger the __get magic method.

FlipEverything commented 4 years ago

To prevent infinite loops. TBH I don't think it really matters.....

sylbru commented 4 years ago

Is it a PHP thing? That __get is not called if the missing property is accessed from within a method of the same object?

I figured preventing infinite loops was the job of the !in_array($this->_inGetter, $field) check in the method, except it wouldn't work as is, if I'm not mistaken (see last paragraph in my original post).

I might also be tired and lost in my own train of thought on this issue… Anyway I just need to remember that I need to use ->get("site") in my methods, but fully understanding the technical reason behind it would help.

FlipEverything commented 4 years ago

I don't know if it's intended or not, but honestly I don't think it's a really big deal. You can debug it if it's really important for you. In the other hand I think that __get should be called for non existent properties....

sylbru commented 4 years ago

Right, I'll move on, thank you for your time!