Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Use ViewableData getters in template engine #2505

Closed
chillu opened this Issue Oct 8, 2013 · 8 comments

Comments

Projects
None yet
6 participants
Owner

chillu commented Oct 8, 2013

The template engine doesn't include arguments when calling methods without their get prefix. One example is FormField->getAttributesHTML():

// works
$getAttributesHTML('class')
// doesn't work
$AttributesHTML('class')

Raised in #2492. Not sure if this has any implications on value caching within the ViewableData layer. I can't figure out how to read or write the SSTemplateParser class, so leaving this to @hafriedlander. Basically, make this test pass:

diff --git a/tests/view/SSViewerTest.php b/tests/view/SSViewerTest.php
index 187bf05..77c9473 100644
--- a/tests/view/SSViewerTest.php
+++ b/tests/view/SSViewerTest.php
@@ -291,6 +291,15 @@ SS;
            . '<% end_if %><% end_loop %><% end_with %>',$data);
        $this->assertEquals("SubKid1SubKid2Number6",$result, "Loop in current scope works");
    }
+
+   public function testGetterWithArguments() {
+       $vd = new SSViewerTest_ViewableData();
+       $this->assertEquals(
+           'arg1:one,arg2:two',
+           SSViewer::fromString('$MethodWithArgs("one","two")')->process($vd),
+           'Accepts arguments with method name leaving out "get" prefix'
+       );
+   }

    public function testObjectDotArguments() {
        $this->assertEquals(
@@ -301,7 +310,7 @@ SS;
                [out:TestMethod(Arg1,Arg2)]
                [out:TestMethod(Arg1).Bar.Val]
                [out:TestMethod(Arg1).Bar]
-               [out:TestMethod(Arg1)]',
+               [out:TestMethod(Arg1)],',
            $this->render('$TestObject.methodWithOneArgument(one)
                $TestObject.methodWithTwoArguments(one,two)
                $TestMethod(Arg1, Arg2).Bar.Val
@@ -1365,6 +1374,10 @@ class SSViewerTest_ViewableData extends ViewableData implements TestOnly {
    public function methodWithTwoArguments($arg1, $arg2) {
        return "arg1:{$arg1},arg2:{$arg2}";
    }
+
+   public function getMethodWithArgs($arg1 = null, $arg2 = null) {
+       return "arg1:{$arg1},arg2:{$arg2}";
+   }
 }

/cc @hafriedlander

Owner

hafriedlander commented Oct 8, 2013

Doesn't really have anything to do with SSViewer, that just calls ViewableData#XML_val (which in turn calls ViewableData#obj which does $value = $this->$fieldName).

However, I don't think this is a bug. Getters can't (and shouldn't) take arguments. If the function needs to take an argument, it should be called AttributesHTML, not getAttributesHTML

This also affects PHP - you can't call echo $foo->AttributesHTML($arg);, you have to do $foo->getAttributesHTML($arg);

Owner

hafriedlander commented Oct 8, 2013

And, no, I didn't notice you'd closed this till after I'd posted that :P.

Owner

chillu commented Oct 8, 2013

@wilr @sminnee What are your thoughts on this? I agree with Hamish that the current behaviour is consistent with PHP, but we've basically been teaching devs for years that in templates, they can just ignore the get prefix, right? I was quite surprised this didn't work, after seven years of using SS templates ;)

Owner

halkyon commented Oct 8, 2013

Heh, I'm with @chillu on this, I actually thought that it would call the getter with the arguments.

Owner

hafriedlander commented Oct 8, 2013

Would you expect if you did $foo->AttributesHTML($arg) that that would work too @halkyon? It's basically the same thing, just not in template land. (I'm pretty sure it's do-able, but it's definitely not how it currently works.)

Owner

hafriedlander commented Oct 8, 2013

There's also the question of what to do if both getAttributesHTML and AttributesHTML exist.

Contributor

stojg commented Oct 9, 2013

I've always been surprised that both are sort of equivalent, but quite often forget things like this. I can't see a reason why it shouldn't be the same.

What's the cons of with / without params?

@simonwelsh simonwelsh added the master label Mar 15, 2014

Contributor

tractorcow commented May 18, 2017

Agreed with @hafriedlander 's original point; Getters shouldn't take arguments, and it's acceptable to force usage of the full method name if arguments are needed.

The getAttributesHTML issue is an example of poor style, but is itself the cause of it's own issues, not the template accessor system.

@tractorcow tractorcow closed this May 18, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment