Silverstripe-framework: Templates ignore arguments in getter methods

[jonom]

Affected Version

4.3.2 (and probably others)

Description

If a controller method is prefixed with get but called in a template without the prefix, arguments passed in the template are not passed to the method.

I'm not sure if this is strictly a bug as I couldn't find a lot of documentation about getters, but my understanding of the convention in SilverStripe is that getMethod() is expected to work the same as Method().

Perhaps by convention getters shouldn't accept arguments but thought this was worth reporting anyhow. Surfaced by this forum post.

Steps to Reproduce

Not prefixed with get

Page controller:

public function Foo() {
    return 'Foo';
}

public function Bar($bar='Bar') {
    return $bar;
}

Page template: $Foo $Bar(Foo)
Expected result: "Foo Foo"
Actual result: "Foo Foo" ✅

Prefixed with get (method only)

Page controller:

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

public function getBar($bar='Bar') {
    return $bar;
}

Page template: $Foo $Bar(Foo)
Expected result: "Foo Foo"
Actual result: "Foo Bar" ❌

Prefixed with get (method and template)

Page controller:

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

public function getBar($bar='Bar') {
    return $bar;
}

Page template: $getFoo $getBar(Foo)
Expected result: "Foo Foo"
Actual result: "Foo Foo" ✅

[lerni]

looks like a duplicate of https://github.com/silverstripe/silverstripe-framework/pull/4768

[jonom]

Ah I thought it must have been discussed somewhere before. I don't think the getter and setter convention/behaviour is adequately documented right now, this is about all I could find: https://docs.silverstripe.org/en/4/developer_guides/model/data_types_and_casting/#overloading I think we could spell things out more definitively and also note that getters shouldn't take arguments.

Might be one of those things that developers working in a big team pick up easily from their colleagues but for those of us working solo getters and setters are a bit of a mysterious concept.

Maybe we should further break down the difference between $this->MyProperty, $this->getMyProperty(), $this->obj('MyProperty'), and $this->dbObject('MyProperty'), what takes precedence and when should you use each one?

Does $this->MyProperty call $this->getMyProperty() if the method exists? And does $this->MyProperty = $value call $this->setMyProperty($value)? I'm embarrassed I don't know the answer to this 😐

[NightJar]

Hi crew,

There was an extensive chat about this on Slack not so long ago (thanks @wernerkrauss for letting me know of this issue).
I agree that the documentation needs to be updated to be very very explicitly clear (yes that level of redundancy :P) about what each "call" is doing.

Long and short of it:

Background

• Think of template scope as if it were an object.
• $Property is for looking up a property (a.k.a. member field). This was the only lookup that existed when SilverStripe templates began (v2).
• In SilverStripe 3 function calls were introduced (i.e. passing of parameters); $Callable("arguments")

Confusion

Because of the second point above, and the 'magic' behind SilverStripe (i.e. in order to get <% control $RelationshipSet %> to work) there is magic fallback functionality in the lookup of a property.
$Property will fall back to getProperty() in order to fetch a pseudo _property_ of an object. This is the magic.

However it is important to distinguish the difference between fetching a property and calling a function:
$getSomething("variable") is _calling_ a function, not fetching a property. As this isn't __get but rather __call as such (loosely put for demonstrative purposes) there is no magic lookup.

It is not that get* is magically looked up on the absence of any template value lookup :)

Conclusion

Is this confusing? It can be sure.
Should it be 'fixed'? I don't really want to 'weigh in' on that, but I would point to "no" as others have before.
Should this be far more clearly documented? Yes, absolutely.

_Let me see if I can look up the slack conversation in the archives_
Starts here, click the timestamp on the last message to advance the paging & read more.
This is probably the simplest summary, if you didn't follow my above explanation.

The conversation goes on with interesting points from @adrexia and @edlinklater discussing the minutiae around impacts of changing, how it's confusing, etc. Probably worth the read if you're passionate about this issue :)

[ScopeyNZ]

I'm inclined to say this is a "wontfix". Magical accessors/getters shouldn't recieve arguments. $getBar(Foo) in your template (ie. explicitly calling functions) would the expected way to do this - and I would argue the clearer way to write your code.

I've tagged this as "feedback-required/core-team" as this is obviously only my opinion 🙂

Edit: I didn't read the issue that was recommended as a duplicate. Looks like it's pretty unanimous that it's not a bug from some long time core contributors so I'll close this. Thanks for starting the discussion though!

[NightJar]

I would argue it stay open and labelled type/docs @ScopeyNZ :)
The duplicate is already long closed.

This is a trap for many a young player I'd expect, particularly those early in their (development) career or dabblers, where the differing concepts between properties and functions isn't entirely clear; so all that remains is confusing syntax that works sometimes and not others.

I'd argue that learning JavaScript at the same time can help conflate those ideas where functions _are_ properties (wew tricky!), so I think the potential for confusion is high enough to warrant a little time spent making things clear in docs. It can only help expand the community in the long run to be accepting of a more diverse developer skill set.

[ScopeyNZ]

I'm happy to review a docs PR. It's probably a fair update to be more specific about the method signature for the magical getters and setters on DataObject. This could probably be achieved by fleshing out the docs that @jonom linked earlier:

https://docs.silverstripe.org/en/4/developer_guides/model/data_types_and_casting/#overloading

[jonom]

Thanks all for the input and sorry for the radio silence at my end (newborn baby at home 😄). Totally agree there's no need to fix this, but as @NightJar said, think it's important to do a docs update. Agree that fleshing out the 'overloading' docs is a good place to do it too.

@NightJar is there any chance you want to take that on? You seem to have a good handle on the problem. If not I will try to do it, it just may be some time before I get around to it (👶=🚫🕙).

p.s. I still have write access here apparently 😄 @chillu dat cool?

[chillu]

p.s. I still have write access here apparently 😄 @chillu dat cool?

Oh, yeah you were set up as an owner. Since you stepped down as a core contributor, I've "demoted" you to a member :) Thanks for checking!