No error thrown when accessing private and protected members of Kirby\Cms\App in a plugin

[hdodov]

Describe the bug
In a plugin, you can access kirby()->roots and no error will be thrown. We had a discussion about that with @distantnative here.

To Reproduce
Steps to reproduce the behavior:

1. Clone the plainkit
2. Create a plugin
3. Put var_dump(kirby()->roots) in the plugin index.php file
4. No error thrown

Expected behavior
An error should be thrown.

Kirby Version
3.3.2

Additional context
If you do var_dump(kirby()->roots) in a template, you correctly get the error:

Cannot access protected property Kirby\Cms\App::$roots

However, doing the same thing in a plugin works, and it shouldn't. @distantnative mentioned this might be due to the way hooks are triggered:

kirby/src/Cms/App.php

Line 1292 in c1ff870

$function->call($this, ...$arguments);

[lukasbestle]

It's because PHP's include() functions (including include_once(), require() and require_once()) import the included file into the current variable scope.

Plugins are loaded from the App class directly, which is why plugins have access to protected methods of the App class.

We could fix this by moving the pluginsLoader() method to a different class or by making it a PHP function, but I wonder if this is worth it. Please note that making the method static will not fix this as even static methods will have access to protected instance methods of the same class. So we really need to move it outside of the App class.

[hdodov]

I think the question then is - should plugins be able to access private and protected members of App? If I build a plugin that relies on the extendTranslations() method, for example, will this plugin work as expected in future Kirby releases?

[lukasbestle]

No, they should not rely on that access. It's a bug that you can access protected methods and properties, but the question is whether it matters: Plugin devs shouldn't use the protected methods for their plugins.

[hdodov]

@lukasbestle devs shouldn't use protected members, that's right. But it should be enforced. If VSCode suggests the extendTranslations() method and I use it, there's no way to know I shouldn't do that - my plugin would work. So even though plugin devs should not use protected members, they can accidentally do it unless they open up the source code. Or even - look at the source code, but not pay attention to the visibility of the member, like I did. I used the method in my plugin, it worked perfectly, and I thought it was ready for release. Then, @distantnative pointed out (in the forum) that this shouldn't have worked.

So yes, I think it does matter. If you have access to something you shouldn't have access to, a fatal error should be thrown.

[lukasbestle]

That's why this issue is currently "missing: discussion 🗣". Waiting for input from @bastianallgeier and @distantnative.

[lukasbestle]

BTW: Same issue for:

• plugins (original issue, https://github.com/getkirby/kirby/blob/3.3.2/src/Cms/AppPlugins.php#L736)
• models (https://github.com/getkirby/kirby/blob/3.3.2/src/Cms/AppPlugins.php#L517)
• collections (https://github.com/getkirby/kirby/blob/3.3.2/src/Cms/Collections.php#L123)
• users (https://github.com/getkirby/kirby/blob/3.3.2/src/Cms/UserActions.php#L262 & https://github.com/getkirby/kirby/blob/3.3.2/src/Cms/Users.php#L113)
• languages (https://github.com/getkirby/kirby/blob/3.3.2/src/Cms/Languages.php#L97)
• Data\PHP (https://github.com/getkirby/kirby/blob/3.3.2/src/Data/PHP.php#L71)
• Toolkit\Component (https://github.com/getkirby/kirby/blob/3.3.2/src/Toolkit/Component.php#L228)
• Toolkit\Controller (https://github.com/getkirby/kirby/blob/3.3.2/src/Toolkit/Controller.php#L58)
• Toolkit\Tpl (https://github.com/getkirby/kirby/blob/3.3.2/src/Toolkit/Tpl.php#L37)
• Toolkit\View (https://github.com/getkirby/kirby/blob/3.3.2/src/Toolkit/View.php#L103)

Maybe we should make a new global helper to load PHP files (basically a helper that runs require_once or require depending on the use-case and returns the result) that can then safely be called from everywhere else.

Edit: We already have that: F::load(). As that method is in a class with no protected members, it should be safe to call that one from everywhere else.

[bastianallgeier]

I agree that we should protect the App class from access via plugins. F::load does a few things that we don't really need here and I would probably prefer to have this as close to a native include as possible.

[bastianallgeier]

✅