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

[RFC] Added new "deprecated" tag #2696

Merged
merged 1 commit into from Jul 31, 2018

Conversation

Projects
None yet
4 participants
@yceruto
Copy link
Contributor

yceruto commented Jun 2, 2018

{% deprecated 'The message...' %}

This new tag would allow us to define a deprecation warning anywhere within a template, useful for Frameworks, bundles, etc. where breaking the BC is frequently when templates are involved.

It will be easy to guarantee a smooth migration path when we want e.g. rename/remove a template or block, as well as displaying an accurate depreciation message.

Deprecating a whole template {# base.twig #}

{% deprecated 'The "' ~ _self ~ '" template is deprecated, use "layout.twig" instead' %}
 
{% extends 'layout.twig' %}

If we're extending from this template {% extends 'base.twig' %} then:

The "base.twig" template is deprecated, use "layout.twig" instead ("base.twig" at line 1).

Deprecating a block {# greeting/blocks.twig #}

{% block hey %}
    {% deprecated 'The "hey" block is deprecated, use "greet" block instead' %}

    {{ block('greet') }}
{% endblock %}

{% block greet %}
    Hey you!
{% endblock %}

If we're using this block {{ block('hey') }} then:

The "hey" block is deprecated, use "greet" block instead ("greeting/blocks.twig" at line 2).

also other examples come from my mind like deprecating macros, and any other extension point.

@yceruto yceruto changed the title [RFC] Added "deprecated" tag [RFC] Added new "deprecated" tag Jun 2, 2018

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch 4 times, most recently from a0f1c66 to 288ffeb Jun 4, 2018

@evelync23

This comment has been minimized.

Copy link

evelync23 commented Jun 5, 2018

A simple way to deprecate things in templates, thank you.

Show resolved Hide resolved doc/tags/deprecated.rst Outdated

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch from 288ffeb to 7b9b8fc Jun 6, 2018

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Jun 6, 2018

should the location of the deprecation (template name and original line) be included automatically in the message ? Otherwise, it might be harder to locate where they come from (the PHP stacktrace will go inside the compiled template).

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch from 7b9b8fc to 4c67fdb Jun 6, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Jun 6, 2018

@stof added template name and template lineno to the message automatically. I like it, thanks.

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch 2 times, most recently from b9d6534 to d8ce240 Jun 6, 2018

Show resolved Hide resolved lib/Twig/Node/Deprecated.php Outdated
Show resolved Hide resolved test/Twig/Tests/Node/DeprecatedTest.php Outdated
Show resolved Hide resolved lib/Twig/Node/Deprecated.php Outdated

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch from d8ce240 to f10b5a5 Jun 6, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Jun 6, 2018

@stof comments addressed.

Show resolved Hide resolved lib/Twig/Node/Deprecated.php Outdated

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch 2 times, most recently from de933ae to 50462a4 Jun 6, 2018

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Jun 7, 2018

this looks fine to me.

I still wonder about 1 thing: if the message is dynamic, any error in building it gets silenced, which might hide mistakes from developers. Here is an idea to improve this:

$compiler->addDebugInfo($this);

$expr = $this->getNode('expr');

if ($expr instanceof Twig_Node_Expression_Constant) {
    $compiler->write('@trigger_error(')
        ->subcompile($expr);
} else {
    $varName = $compiler->getVarName();
    $compiler->write(sprintf('$%s = ', $varName)
        ->subcompile($expr)
        ->raw(";\n")
        ->write(sprintf('@trigger_error($%s', $varName));
}

$compiler
    ->raw('.')
// ...

this way, a dynamic message gets evaluated in a non-silenced statement and used in the next statement (and a constant message avoids the extra variable, so that the concatenation can be resolved by OPCache at compile-time for such case to optimize the code).

what do you think about that @fabpot ?

@yceruto yceruto referenced this pull request Jun 7, 2018

Closed

Deprecate templates #2599

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch from 50462a4 to 1af81a4 Jun 7, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Jun 7, 2018

@stof good catch! I made the changes and I've added a test to cover non-constant expressions.

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Jun 7, 2018

This works fine to deprecate a template. However, it does not work fine to deprecate a block, which is much harder to handle (that's similar to deprecating a protected method in PHP. Warning in the parent method won't catch cases where a child class overrides the method and expects it to keep working). Putting this inside a block will warn for cases where other templates call your block, but not for cases where they override it (there might be ways to trigger such deprecation though, but I haven't tried yet).

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Jun 7, 2018

Right, I'm aware of that limitation but I think that we shouldn't do something to trigger the deprecation of the parent block if it was overridden altogether, just like in the PHP scenario.

For that case, maybe a new argument in block tag could work:

{% block name deprecated 'message' %}...{% endblock %}

Thus, we could look at the parent block and trigger the warning, but that would be out of scope for this PR, so I'd prefer to improve it later :)

This is ready for you? Thank you for all feedbacks :)

@stof

This comment has been minimized.

Copy link
Contributor

stof commented Jun 7, 2018

Thus, we could look at the parent block and trigger the warning, but that would be out of scope for this PR, so I'd prefer to improve it later :)

and that is not possible due to the {% block <name> <expression> %} syntax for blocks.
I agree that this is out of scope of this PR (it might be a matter of combining this tag with some other features btw, as this PR does not add the possibility to deprecate a specific stuff, but to trigger deprecations where you want, including inside a {% if %} to make it conditional)

This is ready for you?

I think it is

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Jul 7, 2018

Bonjour @fabpot, I know your position about this feature in the past #2599 (comment), do you still think this is not a feature that should be provided by Twig directly?

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch from 1af81a4 to 0e8012e Jul 19, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Jul 25, 2018

Hellou? any feedback?

@fabpot
Copy link
Contributor

fabpot left a comment

Can you also add some functional tests (see test/Twig/Tests/Fixtures/tags/spaceless/root_level_in_child.legacy.test for an example)?

You should also add a note in the CHANGES file.

Show resolved Hide resolved doc/tags/deprecated.rst Outdated

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch from 0e8012e to 9806ff8 Jul 31, 2018

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Jul 31, 2018

@fabpot thanks for your feedback, I made the changes.

@yceruto yceruto force-pushed the yceruto:deprecated_tag branch from 9806ff8 to 2ab4338 Jul 31, 2018

@fabpot

This comment has been minimized.

Copy link
Contributor

fabpot commented Jul 31, 2018

Thank you @yceruto.

@fabpot fabpot merged commit 2ab4338 into twigphp:1.x Jul 31, 2018

1 check passed

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

fabpot added a commit that referenced this pull request Jul 31, 2018

feature #2696 [RFC] Added new "deprecated" tag (yceruto)
This PR was merged into the 1.x branch.

Discussion
----------

[RFC] Added new "deprecated" tag

```twig
{% deprecated 'The message...' %}
```

This new tag would allow us to define a deprecation warning anywhere within a template, useful for Frameworks, bundles, etc. where breaking the BC is frequently when templates are involved.

It will be easy to guarantee a smooth migration path when we want e.g. rename/remove a template or block, as well as displaying an accurate depreciation message.

**Deprecating a whole template `{# base.twig #}`**
```twig
{% deprecated 'The "' ~ _self ~ '" template is deprecated, use "layout.twig" instead' %}

{% extends 'layout.twig' %}
```
If we're extending from this template `{% extends 'base.twig' %}` then:
```
The "base.twig" template is deprecated, use "layout.twig" instead ("base.twig" at line 1).
```
**Deprecating a block {# greeting/blocks.twig #}**
```twig
{% block hey %}
    {% deprecated 'The "hey" block is deprecated, use "greet" block instead' %}

    {{ block('greet') }}
{% endblock %}

{% block greet %}
    Hey you!
{% endblock %}
```
If we're using this block `{{ block('hey') }}` then:
```
The "hey" block is deprecated, use "greet" block instead ("greeting/blocks.twig" at line 2).
```
also other examples come from my mind like deprecating macros, and any other extension point.

Commits
-------

2ab4338 Added "deprecated" tag

@yceruto yceruto deleted the yceruto:deprecated_tag branch Jul 31, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment