FIX Don't try to access private properties/methods #10670

GuySartorelli · 2023-01-30T23:25:50Z

A partial solution for this was done in #10637 - this PR makes that solution more robust and does the same for properties.

This also allows ViewableData to fall back to using the $data array if there are private properties instead of just noping out.

Parent issue

ViewableData still tries to use private properties and methods #10669

michalkleiner · 2023-01-31T00:41:25Z

I think we should keep the isPrivateMethod function within ViewableData, similarly to how isPrivateProperty is added.
I'm not sure narrowing down the scope of hasMethod function to skip private methods is necessary here.

emteknetnz

While this may fix the bug, this change doesn't feel quite right

emteknetnz · 2023-01-31T00:47:52Z

src/Core/CustomMethods.php

@@ -147,7 +147,13 @@ protected function registerExtraMethodCallback($name, $callback)
     */
    public function hasMethod($method)
    {
-        return method_exists($this, $method ?? '') || $this->getExtraMethodConfig($method);
+        return (method_exists($this, $method ?? '') && !$this->isPrivateMethod($method)) || $this->getExtraMethodConfig($method);


What happens when you call $this->hasMethod('somePrivateMethod') ... in a $this context the method exists and should be callable?

The other PR looks like it made an assmption that set() had to be non private too, which is also possibly wrong. If I remember correctly we first wanted to use is_callable(), though we couldn't due to the use of __call() making everything callable

Also I think that method would be misnamed with this .. it should be called `hasNonPrivateMethod()'

What happens when you call $this->hasMethod('somePrivateMethod') ... in a $this context the method exists and should be callable?

In what scenario will you define a private method in a class and then in that same class use $this->hasMethod('myPrivateMethod') to see if the method exists? You defined it in that class, of course the method is there. You'd just call it without the condition.

Also I think that method would be misnamed with this .. it should be called `hasNonPrivateMethod()'

I think that's splitting hairs personally. The intention of the method is to see if there's a method there that can be called. If it's private, it can only be called by the class that defined it in which case you wouldn't bother checking if the method is there.
It's only really useful for checking if some external class or this class by virtue of its superclasses defining it has the method.

I feel this change is adding confusing which doesn't need to be there

A quick search in sink for $this->hasMethod yields 26 results, so I think this concern has some merit

How many of those are checking for private methods inside the classes that defined them?
Regardless, I have altered my approach to this.

GuySartorelli · 2023-01-31T00:50:47Z

I think we should keep the isPrivateMethod function within ViewableData, similarly to how isPrivateProperty is added.
I'm not sure narrowing down the scope of hasMethod function to skip private methods is necessary here.

The reason I did it this way is so that $myObj->hasMethod() behaves the same way as $myObj->hasField() - i.e. if it is private, it returns false (for both methods and properties).

The alternative is to do hasMethod() && !isPrivateMethod() (and similarly when checking properties) only in VieweableData. But this could end up meaning people do hasMethod() checks, see true, then try to call the method only for it to be uncallable (and same again with properties).

GuySartorelli · 2023-01-31T00:50:55Z

I have noticed I forgot an important part though - if there's a $data entry and a private property (or a custom extension method and a private method) it still tries to use the private. So I'll need to fix that.

GuySartorelli · 2023-01-31T01:15:35Z

I've reworked the approach to not affect hasMethod() or hasField(). While I think could lead to confusion down the line, it is consistent with the behaviour people have been experiencing until now and I can respect that the consensus seems to be to leave those alone.

michalkleiner · 2023-01-31T01:20:06Z

Thanks Guy. One example I can think of where hasMethod for a private method may make sense is when using traits in project code. You may not have direct visibility of what's in the trait (in terms of looking at the code of a single file) or when a trait is used (as an author of a trait in a 3rd-party module), and there I would expect hasMethod, regardless of its visibility, to always return true. Just a note as you've changed the approach.

emteknetnz · 2023-01-31T01:43:01Z

tests/php/View/ViewableDataTest.php

@@ -208,16 +208,47 @@ public function testSetFailover()
        $this->assertFalse($container->hasMethod('testMethod'), 'testMethod() incorrectly reported as existing');
    }

-    public function testIsPrivate()
+    public function testIsPrivateMethod()


Suggested change

public function testIsPrivateMethod()

public function testIsAccessibleMethod()

emteknetnz · 2023-01-31T01:43:14Z

tests/php/View/ViewableDataTest.php

+        $this->assertTrue($output, 'Method should be accessible');
+    }
+
+    public function testIsPrivateProperty()


Suggested change

public function testIsPrivateProperty()

public function testIsAccessibleProperty()

emteknetnz · 2023-01-31T01:53:52Z

src/View/ViewableData.php

+        if (!method_exists($this, $method)) {
+            return false;
+        }
+        if (static::class === self::class) {


I may (probably am) misunderstanding this, though won't self::class always be ViewableData::class?

Is this what we're trying to do here?

if (!$reflectionMethod->isPrivate()) { return true; } else { // method is private if (<method is defined on the class that called isAccessibleMethod(), or a trait that's been applied to that class>) { return true; } else { return false; } }

won't self::class always be ViewableData::class?

Yes. So we're checking if the class of the object is ViewableData (and not some subclass).

What we're doing is:

Returning false if the method just simply doesn't exist. It's not accessible if it doesn't exist.

Returning true if the method exists and the static class is ViewableData - it can always access its own methods.

Returning false if the method exists, but the static class is not ViewableData, and the method is private. ViewableData can't access methods which are private and defined in subclasses.

Returning true if the method exists, the static class is not ViewableData, but the method is not private. ViewableData can access protected and public methods of its subclasses.

The unit tests cover all of these bases, I believe.

Ah my bad, forgot that isAccessibleMethod() is a private method itself so doesn't need to work on subclasses

emteknetnz

MOG

GuySartorelli · 2023-01-31T02:50:30Z

It's green and has two approvals, so self-merging.

Accidentally broke this in silverstripe#10670

Accidentally broke this in #10670

GuySartorelli mentioned this pull request Jan 30, 2023

ViewableData still tries to use private properties and methods #10669

Closed

GuySartorelli force-pushed the pulls/5/no-private-viewabledata branch from ec52ef6 to b199638 Compare January 30, 2023 23:40

emteknetnz requested changes Jan 31, 2023

View reviewed changes

GuySartorelli force-pushed the pulls/5/no-private-viewabledata branch 2 times, most recently from 9554291 to 934cd75 Compare January 31, 2023 01:15

emteknetnz requested changes Jan 31, 2023

View reviewed changes

FIX Don't try to access private properties/methods

14a449f

GuySartorelli force-pushed the pulls/5/no-private-viewabledata branch from 934cd75 to 14a449f Compare January 31, 2023 01:59

emteknetnz approved these changes Jan 31, 2023

View reviewed changes

michalkleiner approved these changes Jan 31, 2023

View reviewed changes

GuySartorelli merged commit 2274b3e into silverstripe:5 Jan 31, 2023

GuySartorelli deleted the pulls/5/no-private-viewabledata branch January 31, 2023 02:50

GuySartorelli added a commit to creative-commoners/silverstripe-framework that referenced this pull request Feb 1, 2023

FIX Allow public extension getter methods to work

e52d1c2

Accidentally broke this in silverstripe#10670

GuySartorelli mentioned this pull request Feb 1, 2023

FIX Allow public extension getter methods to work #10676

Merged

emteknetnz pushed a commit that referenced this pull request Feb 1, 2023

FIX Allow public extension getter methods to work (#10676)

738ca48

Accidentally broke this in #10670

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX Don't try to access private properties/methods #10670

FIX Don't try to access private properties/methods #10670

GuySartorelli commented Jan 30, 2023

michalkleiner commented Jan 31, 2023

emteknetnz left a comment

emteknetnz Jan 31, 2023

GuySartorelli Jan 31, 2023 •

edited

Loading

GuySartorelli Jan 31, 2023 •

edited

Loading

emteknetnz Jan 31, 2023 •

edited

Loading

GuySartorelli Jan 31, 2023

GuySartorelli commented Jan 31, 2023 •

edited

Loading

GuySartorelli commented Jan 31, 2023

GuySartorelli commented Jan 31, 2023

michalkleiner commented Jan 31, 2023

emteknetnz Jan 31, 2023

GuySartorelli Jan 31, 2023

emteknetnz Jan 31, 2023

GuySartorelli Jan 31, 2023

emteknetnz Jan 31, 2023

GuySartorelli Jan 31, 2023

emteknetnz Jan 31, 2023

emteknetnz left a comment

GuySartorelli commented Jan 31, 2023

	public function testIsPrivateMethod()
	public function testIsAccessibleMethod()

	public function testIsPrivateProperty()
	public function testIsAccessibleProperty()

FIX Don't try to access private properties/methods #10670

FIX Don't try to access private properties/methods #10670

Conversation

GuySartorelli commented Jan 30, 2023

Parent issue

michalkleiner commented Jan 31, 2023

emteknetnz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

GuySartorelli Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

emteknetnz Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuySartorelli commented Jan 31, 2023 • edited Loading

GuySartorelli commented Jan 31, 2023

GuySartorelli commented Jan 31, 2023

michalkleiner commented Jan 31, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emteknetnz left a comment

Choose a reason for hiding this comment

GuySartorelli commented Jan 31, 2023

GuySartorelli Jan 31, 2023 •

edited

Loading

GuySartorelli Jan 31, 2023 •

edited

Loading

emteknetnz Jan 31, 2023 •

edited

Loading

GuySartorelli commented Jan 31, 2023 •

edited

Loading