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

David Grudl

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?

Filip Procházka

~~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 :-(

David Grudl

What about method count()?

Majkl578

I actually never liked nor used that feature.

Filip Procházka

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.

nanuqcz

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)

Vojtěch Dobeš

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)

Lopata

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… :-)

llook

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
}

pekelnik

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)

castamir

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.

Honza Marek

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.

Majkl578

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.

hrach

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().

Jan Tvrdík

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

paranoiq

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

Aurielle

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.

MartinitCZ

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

Filip Procházka

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

llook

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

Vojtěch Dobeš

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)

Lopata

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.

Nox

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.

jasir

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).

Patrik Votoček

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

pekelnik

It's WTF only in PHP world ;)

Milo

What about to prefix/postfix it? Like

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