Magic Methods need PHP Visibility conformance (aka protected setters aren't called by magic method)

[belisoful]

There was recently some confusion about TList/TMap::ReadOnly being protected vs public. I was updating the unit tests for TPriorityMap and discovered that DefaultPriority is being set as a property by magic method when it is protected.

The reason why configuration and templates can access ReadOnly when it is protected is because accessing the property is still accessible by magic methods.

Example.

$map = new TMap();
$map->setReadOnly(true);   // Failure due to being protected
$map->ReadOnly = true; // works because the object constructs and accesses "setReadOnly" on itself.

Is this something to address or not? should it be working this way? Or is this a feature?

Magic methods, canGetProperty, and canSetProperty would need to check the getter/setter method for public.

This is also an issue in Yii2, I think.

[ctrlaltca]

It's definitely something that's breaking basic OOP principles and should be fixed. I'm not sure why, since we should check is_callable() it from the caller context. The real question is: how many things will break if we fix this? :smiling_face_with_tear:

[belisoful]

I don't think this was an error that I introduced when i added behaviors to TComponent; it was something going on before that.

[belisoful]

I'm fairly certain that Yii2 has this problem too because they do a straight method_exists check as well rather than a method_exists then method-is-public check.

I'm going to make a Prado::method_exists in the base utility object that checks if the method is public and then replace method_exists is TComponent with Prado::method_exists

[belisoful]

Processing. one eternity later.... The price of making all properties accessible by magic method to one's own class, is making them publicly available to all. Contrary, Limiting the public to only public getters/setters limits the class itself being able to read its own protected and private properties.

unless the calling object can be found. then when the calling object is $this, then allow the protected/private method call.

This restores OOP principles.

[belisoful]

Here is the function that seems to work better. For external calls, this only allows public, for objects get/set on themselves, it allows protected and private access. This conforms to OOP principles. :)

public static function method_visible($object_or_class, string $method): bool
{
    if (method_exists($object_or_class, $method)) {
        $trace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT | DEBUG_BACKTRACE_IGNORE_ARGS, 3);
        $reflection = new \ReflectionMethod($object_or_class, $method);
        if (empty($trace[2]) || empty($trace[1]['object']) || empty($trace[2]['object']) || $trace[1]['object'] !== $trace[2]['object']) {
            return $reflection->isPublic();
        } elseif ($reflection->isPrivate()) {
            return $trace[2]['class'] === $reflection->class;
        } 
        return true;
    }
    return false;
}

Writing the unit tests... This is the proper logic.

NOTE: I think defining "on" events is ok from protected and private methods. handlers can be added to opaque "on" events but the methods cannot be called. So the "on" events method_exists will not use the new Prado::method_exists.

[belisoful]

I have an idea to resolve the TList ReadOnly being protected from configuration with this correction to magic method visibility to conform to PHP.

Here is my thinking:

• Make TList and TMap setReadOnly public
• Set the default ReadOnly to null
• Allow the first set of ReadOnly as valid (aka when null).
• Deny all other changes to ReadOnly
  • Except when the caller object calling it is the self, child, or parent.

This solution of allowing self, child or parent to change the ReadOnly property and allow all others to set it once is what we are looking for.

The solution is based upon the latest method_accessible function. they would need to retrieve the object of the caller and caller caller, similarly.

I'm thinking of putting a Prado method isCallingSelf and isCallingSelfClass effectively making them "can we call protected and private methods on behalf of the caller" and "can we call private methods on behalf of the caller".

The new public setReadOnly would only allow setting of ReadOnly on first set and by parents, children, and self.

this provides the wanted external functionality vs internal functionality.

[belisoful]

Also... After sitting with method_accessible, which is a better choice, the best name is method_visible because this is about "PHP Visibility" of properties, methods, and constants. I'm going to change the name again. Thankfully it's a minor update.

[belisoful]

After a quick grep "protected function set" -r * there doesn't seem to be any other classes' protected setters that would be used in configuration that would be affected by the visibly change to magic methods. TMap and TList have the one protected ReadOnly property that was being set by magic method from configuration.

There are many protected setters that should be blocked from magic method access. So this is a very good update. Yii team thinks this is an enhancement. I think it's a bug that requires some enhancement.