bobthecow / mustache.php

A Mustache implementation in PHP.
http://mustache.github.io/
MIT License
3.24k stars 420 forks source link

Lambdas do not support complex return types #415

Closed vshih closed 1 month ago

vshih commented 4 months ago
<?php
require 'mustache.php-2.14.2/src/Mustache/Autoloader.php';
Mustache_Autoloader::register();
$m = new Mustache_Engine;

echo $m->render('Hi, {{abc.def}}', ['abc' => function () { return ['def' => 12345]; }]), "\n";

Outputs: Hi,

But I expected: Hi, 12345

bobthecow commented 4 months ago

This is on purpose.

Lambdas don't make sense in the middle of dot notation. If you check the section for dot notation in the Variable Resolution wiki page, you'll see that a lambda in the first part of a dot notation will never render as anything but an empty string. Once the abc part of your tag is resolved, it doesn't invoke the lambda, it checks the lambda itself for a property or method called def.

Given this, the only "valid" things you could have in dot notation after abc are bindTo or call (the two methods on a Closure instance) and those wouldn't even be valid because they wouldn't be called with valid arguments. In fact, checking for methods on Closures is explicitly skipped because it doesn't make sense to resolve variables inside them.

I think the issue might be that you misunderstand what lambdas are, in a Mustache context. They're not a getter, or a way of lazily evaluating code, they're a way of customizing the render of a block (see the docs on Lambdas).

Here's the non-dot-notation equivalent of your example:

$data = [
  'abc' => function () {
    return ['def' => 12345];
  },
];

echo $m->render('Hi, {{# abc }}{{ def }}{{/ abc }}', $data), "\n";

If you run that, it'll give you a hint as to what's going wrong:

PHP Warning:  Array to string conversion in .../NoopCache.php(45) : eval()'d code on line 25
Hi, Array

In other words, it's not traversing into the value you returned, it's trying to echo it. This is because the non-dot-notation form of your template is equivalent to something like this:

echo 'Hi, ', $data['abc']('{{ def }}'), "\n";

… which does the exact same thing, with the same warning.

Knowing what it's doing, we could modify your example to render the right thing:

$data = [
  'abc' => function ($tpl) use ($m) {
    return $m->render($tpl, ['def' => 12345]);
  },
];

echo $m->render('Hi, {{# abc }}{{ def }}{{/ abc }}', $data), "\n";

// or echo 'Hi, ', $data['abc']('{{ def }}'), "\n";
Hi, 12345

… but that's probably not very helpful :)

vshih commented 4 months ago

Thanks for the response.

Well I'm not dreaming up this case, it's mentioned in https://mustache.github.io/mustache.5.html:

Template:

* {{time.hour}}
* {{today}}

Hash:

{
  "year": 1970,
  "month": 1,
  "day": 1,
  "time": function() {
    return {
      "hour": 0,
      "minute": 0,
      "second": 0
    }
  },
  ...

Output:

* 0
* 1970-1-1
bobthecow commented 4 months ago

okay ignore my big long answer. i was thinking just about the section lambda spec, not the interpolation spec. lemme poke at this and get back to you :)

bobthecow commented 4 months ago

Okay so. This is a bug in mustache.php's dotted name resolution. It works as intended with lambdas and non-dotted names, but that doesn't really help when you're trying to access a property returned from the lambda.

And despite clearly being mentioned in the description of lambdas in documentation, dotted name resolution is not actually in the lambdas spec 🙃 I'll get a pull request to add it to the lambdas spec, but in the meantime I can fix Mustache.php's implementation.

vshih commented 4 months ago

Excellent!

bobthecow commented 4 months ago

Okay, this should be fixed in the fix/dotted-lambdas branch. Do you mind confirming with your use case?

vshih commented 4 months ago

Works great so far! I'll continue to test.

vshih commented 4 months ago

Okay, here's another case.

<?php
require 'src/Mustache/Autoloader.php';
Mustache_Autoloader::register();
$m = new Mustache_Engine([
    'pragmas' => [Mustache_Engine::PRAGMA_FILTERS],
]);

$m->addHelper('first_name', function ($array) {
    return $array[0]->name;
});

$names = [
    (object) ['name' => 'Albert'],
    (object) ['name' => 'Betty'],
    (object) ['name' => 'Charles'],
];

echo $m->render(
    "Non-lambda:  {{non_lambda | first_name}}\n"
    . "Lambda:  {{lambda | first_name}}\n",
    [
        'non_lambda' => $names,
        'lambda' => function () use ($names) {
            return $names;
        },
    ]
);

Output:

PHP Warning:  Array to string conversion in src/Mustache/Template.php on line 174

Warning: Array to string conversion in src/Mustache/Template.php on line 174
PHP Warning:  Attempt to read property "name" on string in test.php on line 9

Warning: Attempt to read property "name" on string in test.php on line 9
Non-lambda:  Albert
Lambda:  

The non-Lambda case seems to work fine, but the same data returned via lambda fails.

vshih commented 4 months ago

Maybe this? https://github.com/bobthecow/mustache.php/pull/416

vshih commented 4 months ago

Here's another similar one:

<?php
require 'src/Mustache/Autoloader.php';
Mustache_Autoloader::register();
$m = new Mustache_Engine([
    'pragmas' => [Mustache_Engine::PRAGMA_FILTERS],
]);

$m->addHelper('first_name', function ($array) {
    return $array[0]->name;
});

$names = [
    (object) ['name' => 'Albert'],
    (object) ['name' => 'Betty'],
    (object) ['name' => 'Charles'],
];

echo $m->render(
    "Non-lambda names:\n"
    . "{{#non_lambda}} {{name}}\n{{/non_lambda}}"
    . "Non-lambda names:\n"
    . "{{#lambda}} {{name}}\n{{/lambda}}",
    [
        'non_lambda' => $names,
        'lambda' => function () use ($names) {
            return $names;
        },
    ]
);

Output:

PHP Warning:  Array to string conversion in src/Mustache/Cache/NoopCache.php(45) : eval()'d code on line 31

Warning: Array to string conversion in src/Mustache/Cache/NoopCache.php(45) : eval()'d code on line 31
Non-lambda names:
 Albert
 Betty
 Charles
Non-lambda names:
Array%
bobthecow commented 4 months ago

I've got a fix for that one in https://github.com/bobthecow/mustache.php/tree/fix/dotted-lambdas

vshih commented 4 months ago

Great!

Okay I think I have one more case for you. It's this test case in testLambdaFilters:

            array('{{% FILTERS }}{{# people | first_person }}{{ name }}{{/ people }}', $data, 'Albert'),

But where {{people}} is a lambda which returns $people. If I modify the test data like that, I get:

1) Mustache_Test_FiveThree_Functional_FiltersTest::testLambdaFilters with data set #3 ('{{% FILTERS }}{{# people | fi...ple }}', array(Closure Object (...), Closure Object (...), Closure Object (...), Closure Object (...), Closure Object (...)), 'Albert')
Error: Cannot use object of type Closure as array
bobthecow commented 4 months ago

Thanks! This should be resolved in the branch now.

bobthecow commented 4 months ago

opened a PR to make it a bit easier to reason about :)

vshih commented 4 months ago

Latest works for me. Looks great!

vshih commented 4 months ago

You think this will make it to the next release, and when that might be?

bobthecow commented 4 months ago

It'll be in 2.15.0. As to when that'll be … the last minor version release was December 2021 :P

bobthecow commented 4 months ago

I wanted to also get the bump to spec v1.4.x in that release, but I haven't had the chance to implement that yet.

vshih commented 4 months ago

All righty, thanks for the work.