ObjectAccess uses always 'getProperty' method when accessed object has '__call' magic method

[uris-dev]

Description

If an (repository) object has the __call magic method, then all methods are automatically callable, and ObjectAccess tries to retrieve the value always with the get[PropertyName] method, instead of the is[PropertyName] or has[PropertyName]
I encountered the problem when using the TranslatableTrait for RepositoryObjects

I'm not sure if this is the intended behavior, but it effectively prevents the reliable use the 'is' and 'has' prefix as property getters. The problem can be fixed adding an additional check in initializePropertyGetterCache():
from
if (is_callable([$subject, $getterMethodName])) {
to
if (method_exists($subject, $getterMethodName) && is_callable([$subject, $getterMethodName])) {

Steps to Reproduce

1. add (boolean) property blah = true and getter isBlah() to an object
2. Add __call magic method to object where ObjectAccess should retrieve a property value
3. Call ObjectAccess::getProperty($object, 'blah');

Expected behavior

the call should return true

Actual behavior

it returns null, since it tries to get the value via getBlah()

Affected Versions

all versions using the actual repository

[kitsunet]

Interesting, and good catch. The suggested solution breaks magic getters though, (using __call to emulate getters). We need to think carefully which we want :)

[uris-dev]

Yes, I realized that backward compatibility is a concern to such a central operation, the following code could alleviate these concerns, but not prevent breaking code in edge cases that rely on the actual behavior (like having an isBlah() method but expecting a different value from a __call() method):

// first test for real methods
foreach ($getterMethodNames as $getterMethodName) {
    if (method_exists($subject, $getterMethodName) && is_callable([$subject, $getterMethodName])) {
        self::$propertyGetterCache[$cacheIdentifier]['accessorMethod'] = $getterMethodName;
        return;
    }
}

// further test for a potential magic __call method (it's the task of the magic method to handle the call properly)
if(is_callable([$subject, $getterMethodNames[0]])) {
    self::$propertyGetterCache[$cacheIdentifier]['accessorMethod'] = $getterMethodNames[0];
    return;
}

[bwaidelich]

I think it would be fine to require objects to implement \ArrayAccess if they want sth like "magic getters" (shudder).
But that would be a breaking change and thus probably only viable for a major release

[uris-dev]

Ok, I recognize that the use of magic getters is not such a good practice in a model and in the concrete case that I faced I realized that we can just remove that from our code (since the __call method in a trait does not come from an external library as I originally thought).
Regarding the requirement to implement \ArrayAccess, how would it work? Would the framework throw if I use __call without \ArrayAccess, or how would the user realize something is missing?
Or maybe should the framework just throw if you use a magic method in a model?

[bwaidelich]

Regarding the requirement to implement \ArrayAccess

You would just implement the four methods of the ArrayAccess interface and then use them like

$yourObject['yourProperty'];

and/or

ObjectAccess::getProperty($yourObject, 'yourProperty');

Or maybe should the framework just throw if you use a magic method in a model?

I don't think so – there are other use cases for magic methods (even though I'd vote to get rid of them if its an option)