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

API: Preserve variable types in template lookup args (closes #4822) #8152

Merged
merged 1 commit into from
Jun 12, 2018

Conversation

kinglozzer
Copy link
Member

No description provided.


# Integers and floats

IntOrFloat: / (\d+(\.\d+)?) /
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh. This needs to be updated to support passing things like .5, -10 etc. There are other expressions that are valid numbers in PHP (exponents, hexadecimal, binary), not sure if the effort/reward will be worth it for those - I’ll look into it

@kinglozzer kinglozzer changed the title API: Preserve variable types in template lookup args (closes #4822) [Don’t merge] API: Preserve variable types in template lookup args (closes #4822) Jun 8, 2018
@@ -286,7 +286,8 @@ class SSTemplateParser extends Parser implements TemplateParser

$property = $sub['Call']['Method']['text'];

if (isset($sub['Call']['CallArguments']) && $arguments = $sub['Call']['CallArguments']['php']) {
if (isset($sub['Call']['CallArguments']) && isset($sub['Call']['CallArguments']['php'])) {
$arguments = $sub['Call']['CallArguments']['php'];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change required as sometimes the arguments might simply be an int 0 or boolean false

@@ -412,7 +412,7 @@ public function renderWith($template, $customFields = null)
protected function objCacheName($fieldName, $arguments)
{
return $arguments
? $fieldName . ":" . implode(',', $arguments)
? $fieldName . ":" . var_export($arguments, true)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required as we can now pass actual booleans as arguments. Implode will cast false to an empty string, which would mean the same cache name between $Foo and $Foo(false)

@kinglozzer kinglozzer changed the title [Don’t merge] API: Preserve variable types in template lookup args (closes #4822) API: Preserve variable types in template lookup args (closes #4822) Jun 12, 2018
@dhensby
Copy link
Contributor

dhensby commented Jun 12, 2018

wow - this is cool, I love it. So if you now want the string "true" it needs to be quoted in the template, right?

I guess there's a distinction between "1" and 1 too?

One other feature I'd like to see is the template throwing warnings or errors if there's some kind of unquoted string - would that be much work?

@kinglozzer
Copy link
Member Author

So if you now want the string "true" it needs to be quoted in the template, right?
I guess there's a distinction between "1" and 1 too?

Correct on both counts

One other feature I'd like to see is the template throwing warnings or errors if there's some kind of unquoted string - would that be much work?

I think that should be pretty simple, we could trigger a deprecation notice or something. The counter argument is that as long as it’s not blocking any new features we want to add, why bother removing support for it? I personally prefer to be explicit about strings (and the same with remembering the $ in <% loop $Foo %>), but I’m sure there are others out there that prefer the safety net of not having to worry about it.

@dhensby
Copy link
Contributor

dhensby commented Jun 12, 2018

I guess I like the idea of being able to define a strict template syntax and it feels like that's the way we are heading anyway...

@tractorcow
Copy link
Contributor

@kinglozzer do you want to be the new ss template engine sage? Because that's what it sounds like. ;) Somewhere a snowy mountain top is awaiting your presence.

@tractorcow tractorcow merged commit bf86c8e into silverstripe:master Jun 12, 2018
@kinglozzer
Copy link
Member Author

5291762

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 this pull request may close these issues.

3 participants