Skip to content

[Twig] add computed properties system #266

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

Merged
merged 1 commit into from
Feb 18, 2022

Conversation

kbond
Copy link
Member

@kbond kbond commented Feb 3, 2022

Q A
Bug fix? no
New feature? yes
Tickets n/a
License MIT

This adds a computed component template variable to make "computed methods" easier. In your component's template, to call a method on your component and cache the return (to use again later), instead of calling this.methodName, call computed.methodName:

{# templates/components/featured_products.html.twig #}

<div>
    <h3>Featured Products</h3>

    {% for product in computed.products %}
        ...
    {% endfor %}

    ...
    {% for product in computed.products %} {# use cache, does not result in a second query #}
        ...
    {% endfor %}
</div>

@kbond kbond mentioned this pull request Feb 3, 2022
1 task
@kbond kbond changed the title [RFC][Twig] add computed method system [Twig] add computed method system Feb 4, 2022
@kbond
Copy link
Member Author

kbond commented Feb 4, 2022

Should we make the public properties available via computed (computed.prop1)? Right now only public methods are available.

@weaverryan
Copy link
Member

Should we make the public properties available via computed (computed.prop1)? Right now only public methods are available.

Ideally yes... just so that this and component work identically. But more so, could we re-use the Twig logic for calling the method / fetching the property? For example, we know that Twig checks several things when we say this.foo to get that property / call that method. Can we reuse that inside the proxy class?

@kbond
Copy link
Member Author

kbond commented Feb 4, 2022

Can we reuse that inside the proxy class?

I believe so, yes. I only check get/no-get method prefix now (so computed.getMethod and computed.method both work and refer to the same cache). I'll take a look at twig's internals and try to match.

@kbond
Copy link
Member Author

kbond commented Feb 11, 2022

Ok, I think this is ready. I ensure computed.x works with public properties, components implementing \ArrayAccess, getter's, isser's and hasser's.

@kbond kbond requested a review from weaverryan February 11, 2022 14:27
@kbond kbond changed the title [Twig] add computed method system [Twig] add computed properties system Feb 11, 2022
Copy link
Member

@weaverryan weaverryan left a comment

Choose a reason for hiding this comment

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

This looks great! i just wanted to triple-check about reusing the Twig logic

if (method_exists($this->component, $method = sprintf('%s%s', $prefix, ucfirst($name)))) {
return $method;
}
}
Copy link
Member

Choose a reason for hiding this comment

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

So I'm guessing there wasn't a nice way to "steal" and re-use the Twig native functionality?

Copy link
Member Author

Choose a reason for hiding this comment

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

There was not...

Copy link
Member Author

Choose a reason for hiding this comment

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

I looked at using twig_get_attribute but it requires injecting the twig environment and Source.

Copy link
Member

Choose a reason for hiding this comment

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

Wow, Source, that's a new class to me :)

@kbond
Copy link
Member Author

kbond commented Feb 14, 2022

Rebased and conflicts resolved.

@weaverryan
Copy link
Member

Thank you Kevin!

@weaverryan weaverryan merged commit fcf9444 into symfony:2.x Feb 18, 2022
@kbond kbond deleted the computed-methods branch February 18, 2022 00:48
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.

2 participants