Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lambdas do not support complex return types #415

Closed
vshih opened this issue May 9, 2024 · 19 comments · Fixed by #417
Closed

Lambdas do not support complex return types #415

vshih opened this issue May 9, 2024 · 19 comments · Fixed by #417

Comments

@vshih
Copy link
Contributor

vshih commented May 9, 2024

<?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
Copy link
Owner

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
Copy link
Contributor Author

vshih commented May 9, 2024

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
Copy link
Owner

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
Copy link
Owner

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
Copy link
Contributor Author

vshih commented May 9, 2024

Excellent!

bobthecow added a commit that referenced this issue May 10, 2024
@bobthecow
Copy link
Owner

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

@vshih
Copy link
Contributor Author

vshih commented May 10, 2024

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

vshih added a commit to myersmediagroup/mustache.php that referenced this issue May 10, 2024
vshih added a commit to myersmediagroup/mustache.php that referenced this issue May 10, 2024
@vshih
Copy link
Contributor Author

vshih commented May 11, 2024

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
Copy link
Contributor Author

vshih commented May 12, 2024

Maybe this? #416

@vshih
Copy link
Contributor Author

vshih commented May 12, 2024

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
Copy link
Owner

bobthecow commented May 13, 2024

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

bobthecow added a commit that referenced this issue May 13, 2024
@vshih
Copy link
Contributor Author

vshih commented May 13, 2024

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
Copy link
Owner

Thanks! This should be resolved in the branch now.

@bobthecow
Copy link
Owner

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

@vshih
Copy link
Contributor Author

vshih commented May 13, 2024

Latest works for me. Looks great!

@vshih
Copy link
Contributor Author

vshih commented May 14, 2024

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

@bobthecow
Copy link
Owner

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

@bobthecow
Copy link
Owner

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
Copy link
Contributor Author

vshih commented May 15, 2024

All righty, thanks for the work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants