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

Add "filter", "map", and "reduce" filters #2996

Merged
merged 5 commits into from May 9, 2019

Conversation

8 participants
@fabpot
Copy link
Contributor

commented May 3, 2019

This PR adds support for 3 new filters: filter, map, and reduce. They take an arrow function as an argument (a PHP closure).

I have restricted the usage of arrow functions as much as possible as it makes no sense to support them everywhere. So, for now, they are only accepted as arguments to filters (using them as arguments to function is not supported but could be easily added if we have a use case).

The syntax is the following: (x) => x + 3 where (x) is the list of arguments, => starts the body, and the body is any Twig expression. Within the arrow function, the context is also available: (x) => x + offset works if offset is defined in the current context.

These new filters will allow us to deprecate the if support on the for tag, which does not work well with the loop variable:

{% set sizes = {xs: 34, s: 36, m: 38, l: 40, xl: 42} %}

{# before #}
{% for name, size in sizes if size < 38 %}
    {{ name }} = {{ size }}
    {% loop.last ? 'LAST' %} {# <--- works with this PR #}
{% endfor %}

{# after #}
{% for name, size in sizes|filter(size => size < 38) %}
    {{ name }} = {{ size }}
    {{ loop.last ? 'LAST' }}
{% endfor %}

This closes #2785

@hason

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

The if on the for tag should be preserved as syntactic sugar and internally should be converted to the filter.

@hason

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

What about to pass both value and key as arguments to filter arrow function (see ARRAY_FILTER_USE_BOTH):

{% for name, size in sizes|filter(|size[, name]| => name != '...') %}
Show resolved Hide resolved src/Token.php Outdated
Show resolved Hide resolved src/Node/Expression/ArrowFunctionExpression.php Outdated
@javiereguiluz

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Don't know if it's technically possible (or desirable) but the |x| => ... syntax looks like Ruby to me. I fear PHP developers could find this syntax strange.

Could we use the just approved and implemented (x) => ... syntax of PHP arrow functions?

{% for name, size in sizes|filter((size) => size < 38) %}
    {{ name }} = {{ size }}
    {{ loop.last ? 'LAST' }}
{% endfor %}
@fabpot

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@javiereguiluz I chose |a| => for a good reason :) It makes parsing way more easier. Using (a) => means that we have parsing ambiguities as () have already a meaning. It's far from impossible, but I'd like to avoid adding too much complexity for such a feature.

@linaori

This comment has been minimized.

Copy link

commented May 3, 2019

I find |foo| very confusing, not because it's ruby, but because | means that it's a filter over something. If I see the example: {% for name, size in sizes|filter(|size| => size < 38) %, I have a lot of issues seeing what this is as I'm used to treat | as a filter over the left side. Reading this, it looks like you're trying to do: sizes | filter( | size | => size, which looks really weird to me

@fabpot fabpot force-pushed the fabpot:arrow-functions branch 2 times, most recently from 7f4c3da to ce1656e May 3, 2019

@fabpot

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@hason I have changed the condition implementation of the for tag to remove the limitation (but we need to now convert the iterated var to an array).

@fabpot

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

I will see if we can use the more natural (x) => x + 1 without too many changes.

@jfcherng

This comment has been minimized.

Copy link

commented May 3, 2019

How about using the same syntax with PHP 7.4's array function? The RFC implementation has been merged into the PHP 7.4 branch already.

fn(x) => x + 1

The preceding fn makes it much easier to parse.

@hason

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@hason I have changed the condition implementation of the for tag to remove the limitation (but we need to now convert the iterated var to an array).

It would be useful to support iterators CallbackFilterIterator.

@fabpot

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@hason I'm going to remove the code for the for tag from this PR and create another one after merging the main feature of this PR.

@fabpot fabpot force-pushed the fabpot:arrow-functions branch from ce1656e to 4375ffd May 3, 2019

@fabpot

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Good news, I've changed the syntax to (x) => x + 1!

@fabpot fabpot force-pushed the fabpot:arrow-functions branch from abefecf to c53056e May 3, 2019

@fabpot

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

Now with the possibility to omit the () when having only one argument: x => x + 1

@fabpot fabpot force-pushed the fabpot:arrow-functions branch from c53056e to 05702f1 May 3, 2019

@stof

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

should we allow arrow functions only in filter arguments ? What about function arguments, test arguments or custom calls ?

@willrowe

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

Would it be possible to pass the key as a second argument for filter and map and as a third argument for reduce?

filter((value, key) => ...)
map((value, key) => ...)
reduce((carry, value, key) => ...)
@fabpot

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@stof I don't want to expose arrow function without a good use case. For now, we only need them for functions, so that's why I've only enabled them there.

@fabpot fabpot force-pushed the fabpot:arrow-functions branch from 05702f1 to bf6ef23 May 3, 2019

@fabpot fabpot force-pushed the fabpot:arrow-functions branch from bf6ef23 to 13274bb May 3, 2019

@fabpot

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

@willrowe Done for filter and map, not sure it makes sense for reduce (I don't see a use case).

@fabpot fabpot force-pushed the fabpot:arrow-functions branch from aacc998 to 5bef112 May 3, 2019

@fabpot

This comment has been minimized.

Copy link
Contributor Author

commented May 3, 2019

The feature looks "finished" to me now.

@fabpot fabpot force-pushed the fabpot:arrow-functions branch 2 times, most recently from 0e48d64 to 6bf7fa2 May 3, 2019

@fabpot fabpot force-pushed the fabpot:arrow-functions branch from 6bf7fa2 to 5c15f89 May 3, 2019

@fabpot fabpot merged commit 5c15f89 into twigphp:1.x May 9, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

fabpot added a commit that referenced this pull request May 9, 2019

feature #2996 Add "filter", "map", and "reduce" filters (fabpot)
This PR was squashed before being merged into the 1.x branch (closes #2996).

Discussion
----------

Add "filter", "map", and "reduce" filters

This PR adds support for 3 new filters: `filter`, `map`, and `reduce`. They take an arrow function as an argument (a PHP closure).

I have restricted the usage of arrow functions as much as possible as it makes no sense to support them everywhere. So, for now, they are only accepted as arguments to filters (using them as arguments to function is not supported but could be easily added if we have a use case).

The syntax is the following: `(x) => x + 3` where `(x)` is the list of arguments, `=>` starts the body, and the body is any Twig expression. Within the arrow function, the context is also available: `(x) => x + offset` works if `offset` is defined in the current context.

~~These new filters will allow us to deprecate the `if` support on the `for` tag, which does not work well with the `loop` variable:~~

```twig

{% set sizes = {xs: 34, s: 36, m: 38, l: 40, xl: 42} %}

{# before #}
{% for name, size in sizes if size < 38 %}
    {{ name }} = {{ size }}
    {% loop.last ? 'LAST' %} {# <--- works with this PR #}
{% endfor %}

{# after #}
{% for name, size in sizes|filter(size => size < 38) %}
    {{ name }} = {{ size }}
    {{ loop.last ? 'LAST' }}
{% endfor %}
```

This closes #2785

Commits
-------

5c15f89 added the key to the map and filter filters
13274bb added support for iterators
175041e removed fn in front of arrow functions
dc27763 changed arrow syntax
b30bce1 added "filter", "map", and "reduce" filters

@fabpot fabpot deleted the fabpot:arrow-functions branch May 14, 2019

fabpot added a commit that referenced this pull request May 20, 2019

minor #3024 doc(filter): add example of `filter` without `for` use (K…
…ocal)

This PR was submitted for the 2.x branch but it was merged into the 1.x branch instead (closes #3024).

Discussion
----------

doc(filter): add example of `filter` without `for` use

First, thanks for #2996! :)

This PR add an example of `filter` usage without using a `for` loop, to prevend some misinterpretation to people.

At first read, I thought `filter` was only usable with `for` loop, but nope. In fact it iterates on the result produced by `filter`.

Commits
-------

847276a doc(filter): add example of `filter` without `for` use
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.