Damn feature: access to $object->method as callback

6 years ago

David Grudl
Nette Core | 6864
+
0
-

One year ago I have added this cool feature ObjectMixin: added support for getting public methods as Closure:

class TestClass extends Nette\Object
{
    public function adder($a, $b)
    {
        return $a + $b;
    }
}

$obj = new TestClass;
$method = $obj->adder;
echo $method(2, 3); // 5

It moves PHP towards JavaScript/Python.

It's not just “theoretical” feature, it has real usage, i.e in sandbox.

But I have found a damn disadvantage. See this code:

if ($user->isLoggedIn) {
    echo 'sensitive data';
}

The condition is always true because there is a typo, missing brackets. So $user->isLoggedIn returns callback for method isLoggedIn().

What to do? Remove this feature?

6 years ago

Filip Procházka
Moderator | 4693
+
0
-

Check if method starts with is, get or set (those that Nette\Object transfers to properties), and blacklist those prefixes for callbacks.

Don't remove the feature :-(

6 years ago

David Grudl
Nette Core | 6864
+
0
-

What about method count()?

6 years ago

Majkl578
Moderator | 1379
+
0
-

I actually never liked nor used that feature.

6 years ago

Filip Procházka
Moderator | 4693
+
0
-

David Grudl: Sadly, good point. You're obviousely decided to remove it. So, we basically have theese options

  • make it optional
  • leave it only on presenters/controls/forms for bc

Can we at least still have it as a trait at least? Or something like that.

6 years ago

nanuqcz
Member | 844
+
0
-

David Grudl wrote:

It moves PHP towards JavaScript/Ruby/Python.

How is this problem solved in JavaScript/Ruby/Python?

Last edited by nanuqcz (2013-04-06 18:30)

6 years ago

Vojtěch Dobeš
Member | 1317
+
0
-

Damn, I love this feature! It's so elegant… What about trigger warning when the method is called the same as property? Because that's always the problematic case. And usually the code is even more readable when there are no properties called the same as methods (eg. because of code completion).

@nanuqcz In Javascript I don't think this is solved somehow.

Last edited by vojtech.dobes (2013-04-06 19:48)

6 years ago

Lopata
Member | 139
+
0
-

Well, it's not so much of a big deal if it took an entire year for anyone to notice, is it? I find the example code illustrating this vulnerability rather funny because I honestly cannot imagine an application with such mistake passing basic (even non-automated) tests and being subsequently deployed to a production environment. Having considered how callbacks are used in Nette-based projects (they usually handle rather important tasks), I just think it would be extremely hard not to notice a flaw like that. Of course, there might be extreme cases where this could happen but from an utilitarian standpoint I must conclude that they are absolutely outweighed by the awesomeness of this feature and the comfort it brings to everyone else.

And just for the record, as far as I know, Javascript does not do anything about it whatsoever. It almost seems as if programmers working with C-family languages were somehow expected to know when and how to use parentheses… :-)

6 years ago

llook
Member | 412
+
0
-

I think this problem exists mainly in templates (as there is no IDE with proper support of Latte) and mainly with boolean values. Values of other types are usually compared with strict operators and if some cycle in template runs 0 times instead of ->count() times, I don't see it as so big deal.

Blacklisting some prefixes is a way to more entropy. I wouldn't go this way, I suggest to either include or remove this feature. Question states: Do you really want to move PHP towards JavaScript/Ruby/Python?

I think callback($this, 'signInFormSucceeded') is more expressive, than $this->signInFormSucceeded and expressivity is usually a good thing in programming.

What about trigger warning when the method is called the same as property? Because that's always the problematic case.

No, the case is when someone types $this->isLoggedIn, he gets no warning on unexistent property and so he thinks it works.

Maybe it could always trigger an error (not an exception). So when you use the feature intentionally, you add an @. If you use it by mistake, you get an error:

$form->onSuccess[] = @$this->signInFormSucceeded; // OK
if ($user->isLoggedIn) { // error
}

6 years ago

pekelnik
Member | 468
+
0
-
if (@$user->isLoggedIn)

LOL

I would keep this feature intact. It is awesome! Another discussion can be about the breakage of the Nette\Object to some traits.

I think the only disadvantage of this feature is that it is not native in PHP ;)

Last edited by pekelnik (2013-04-06 20:53)

6 years ago

castamir
Member | 631
+
0
-

I don't use it very often, maybe only with forms, so it wont't hurt me so much, but on the other hand I dont't think it is such a problem. As @Lopata wrote, there wasn't any sign of a problem for the entire year so far. Keep it as it is or remove it entirely. I prefer the first one. If a coder has a good IDE for coding (like PHPStorm), he will noticed by a message that something might be wrong. In case of PHPStorm it is a message like “an undefined or a magic property usage”.

There is no other choices, because half-solutions (like blackisting set, get, is and on) will be confusing for everyone.

6 years ago

Honza Marek
Member | 1674
+
0
-

For me it is most exciting feature in whole Nette\Object. And with popular feature – Nette\Object properties – you can easily write a typo too.

class Example
{
    private $foo = 'xxx';

    public function getFoo()
    {
        return 'yyy';
    }

    public function someMethod()
    {
        // what does $val contain?
        $val = $this->foo;
    }
}

So there is no reason for removing this cool feature.

6 years ago

Majkl578
Moderator | 1379
+
0
-

Honza Marek wrote:

And with popular feature – Nette\Object properties – you can easily write a typo too.

That's not a typo, that's logical mistake.

6 years ago

hrach
Member | 1817
+
0
-

I love this feature and I never have troubles with it. Let it be, please :)

Count is much more complicated example of fucked php design when \Countable needs this method, so there is no possibility to implement different behaviour for ->count().

6 years ago

Jan Tvrdík
Nette guru | 2559
+
0
-

I personally never like this feature because it makes the code which use it harder to read.

6 years ago

paranoiq
Member | 388
+
0
-

i don't like it. it does not say what it does

6 years ago

Aurielle
Member | 1283
+
0
-

I actually didn't know that this feature existed and therefore never used it. But I have to agree with @Jan Tvrdlík and @paranoiq, because when I was looking at the linked example in sandbox, it didn't occur to me for a minute or so. I just didn't knew what it does, as it seems that it can bring more problems than code improvements.

6 years ago

MartinitCZ
Member | 590
+
0
-

@**Jan Tvrdík** & @**paranoiq**: +1 … Delete it.

6 years ago

Filip Procházka
Moderator | 4693
+
0
-

@martinit it cannot be just deleted. There has to be backwards compatibility.

6 years ago

llook
Member | 412
+
0
-

It can be deleted in @dev and in the next major version (with DeprecatedException).

6 years ago

Vojtěch Dobeš
Member | 1317
+
0
-

Actually described trouble has never happened to me before, but I experienced different one:

$this->onMyEvent = $this->firstCallback;
$this->onMyEvent = $this->secondCallback;

Only the second callback got fired. Why? Missing [], which is exactly the same amount of characters as in case describer in this thread. I don't see this as a reason to remove or change current implementation of events in Nette\Object, and so I don't see a reason to remove or change this feature.

My example is so confusing, because it's I usually copy the first callback assignment. And it works correctly with just one.

Last edited by vojtech.dobes (2013-04-07 20:09)

6 years ago

Lopata
Member | 139
+
0
-

The feature is just as confusing and hard to read as

<?php
$val = $obj->foo; // calls $obj->getFoo();
?>

Nette is absolutely based on similar sorcery. The only difference is that we are all used to reading it and know what it actually does. Should we decide do remove this feature, the reason should not be what @Jan Tvrdík and @paranoiq suggest as that would be an application of a double standard.
As mentioned above, those of us who have actually tried using it, think that it is rather nice and handy. Consequently, I believe that this is more of a matter of lack of open mindedness rather than an objectively assessable issue with readability.

6 years ago

Nox
Member | 381
+
0
-

Well the only reason not to question the stay of magic g/setters is that it's probably too rooted in apps and would be too big a blow to BC.
I personally refrain from using any of the mentioned magic, harder to find out what it really does, does not work well with IDE, slows down application and in this case can be another source of errors. But MemberAccessException is great, I'd vote for keeping that one.

6 years ago

jasir
Member | 748
+
0
-

Keep the feature, it is awesome. But there should be an information about this in the documentation (which is not now, or I have searched badly).

6 years ago

Patrik Votoček
Member | 2249
+
0
-

This feature is most WTF Nette feature I know. But no one tell you “you must use it”.

6 years ago

pekelnik
Member | 468
+
0
-

It's WTF only in PHP world ;)

6 years ago

Milo
Nette Core | 1149
+
0
-

What about to prefix/postfix it? Like

$this->isLoggedInCallback
$this->isLoggedInMethod
$this->cbIsLoggedIn