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 test examples for include to render #327

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

alexander-schranz
Copy link
Contributor

@alexander-schranz alexander-schranz commented Nov 8, 2024

This is just some example test case how we may could migrate current include to a new render method or whatever it will be called.

twigphp/Twig#4434 (still in discussion)

@alexander-schranz alexander-schranz changed the title Add test examples for inclue to render Add test examples for include to render Nov 8, 2024
Copy link
Owner

@VincentLanglet VincentLanglet left a comment

Choose a reason for hiding this comment

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

You should add example for

{{ include('template.html', with_context = false) }}

{{ render('template.html', {'foo': 'bar'}|merge(_context)) }}
{{ render('template.html', {'foo': 'bar'}|merge(_context), true) }}
{{ render('template.html', {'foo': 'bar'}|merge(_context), true, true) }}
{{ render('template.html', with_context = true) }}
Copy link
Owner

Choose a reason for hiding this comment

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

If I understand the render method, there is no with_context param.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes fixed it and added with_context = false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The changes are still in discussion but wanted to show example how fixer could make a upgrade possible.

@smnandre
Copy link
Contributor

smnandre commented Nov 9, 2024

If you want to bootstrap the rule, this should be a good start:

src/Rules/Function/IncludeToRenderFunctionRule.php

<?php

declare(strict_types=1);

namespace TwigCsFixer\Rules\Function;

use TwigCsFixer\Rules\AbstractFixableRule;
use TwigCsFixer\Token\Token;
use TwigCsFixer\Token\Tokens;

/**
 * Ensures that render function is used instead of deprecated include function.
 */
final class IncludeToRenderFunctionRule extends AbstractFixableRule
{
    protected function process(int $tokenIndex, Tokens $tokens): void
    {
        $token = $tokens->get($tokenIndex);
        if (!$token->isMatching(Token::FUNCTION_NAME_TYPE, 'include')) {
            return;
        }

        $fixer = $this->addFixableError(
            'Render function must be used instead of include function.',
            $token
        );
        if (null === $fixer) {
            return;
        }

        // ...

        $fixer->replaceToken($tokenIndex, 'render');

        // ...
    }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Minor detail, but this file shoud be named IncludeToRenderFunctionRuleTest.fixed.twig (the Test suffix is missing)

tests/Rules/Function/IncludeToRender/IncludeToRenderFunctionRuleTest.fixed.twig

@smnandre
Copy link
Contributor

smnandre commented Nov 9, 2024

Not sure if all would be necessary, but here are the list of tests to ensure named arguments (with :, don't think we need the same with =

{{ include('template.html', ignore_missing: true) }}
{{ include('template.html', ignore_missing: true, with_context: false) }}
{{ include('template.html', ignore_missing: true, with_context: true) }}
{{ include('template.html', ignore_missing: true, with_context: false, sandboxed: true) }}
{{ include('template.html', ignore_missing: true, with_context: true, sandboxed: true) }}
{{ include('template.html', ignore_missing: true, sandboxed: true) }}
{{ include('template.html', ignore_missing: true, sandboxed: true, with_context: true) }}
{{ include('template.html', ignore_missing: true, sandboxed: true, with_context: false) }}

{{ include('template.html', sandboxed: true) }}
{{ include('template.html', sandboxed: true, with_context: true) }}
{{ include('template.html', sandboxed: true, with_context: false) }}
{{ include('template.html', sandboxed: true, with_context: true, ignore_missing: true) }}
{{ include('template.html', sandboxed: true, with_context: false, ignore_missing: true) }}
{{ include('template.html', sandboxed: true, ignore_missing: true) }}
{{ include('template.html', sandboxed: true, ignore_missing: true, with_context: true) }}
{{ include('template.html', sandboxed: true, ignore_missing: true, with_context: false) }}

{{ include('template.html', with_context: true) }}
{{ include('template.html', with_context: true, sandboxed: true) }}
{{ include('template.html', with_context: true, sandboxed: true, ignore_missing: true) }}
{{ include('template.html', with_context: true, ignore_missing: true) }}
{{ include('template.html', with_context: true, ignore_missing: true, sandboxed: true) }}

{{ include('template.html', with_context: false) }}
{{ include('template.html', with_context: false, sandboxed: true) }}
{{ include('template.html', with_context: false, sandboxed: true, ignore_missing: true) }}
{{ include('template.html', with_context: false, ignore_missing: true) }}
{{ include('template.html', with_context: false, ignore_missing: true, sandboxed: true) }}

And the fixed equivalent

{{ render('template.html', _context, ignore_missing: true) }}
{{ render('template.html', ignore_missing: true) }}
{{ render('template.html', _context, ignore_missing: true) }}
{{ render('template.html', _context, ignore_missing: true, sandboxed: true) }}
{{ render('template.html', _context, ignore_missing: true, sandboxed: true) }}
{{ render('template.html', _context, ignore_missing: true, sandboxed: true) }}
{{ render('template.html', _context, ignore_missing: true, sandboxed: true) }}
{{ render('template.html', ignore_missing: true, sandboxed: true) }}

{{ render('template.html', _context, sandboxed: true) }}
{{ render('template.html', _context, sandboxed: true) }}
{{ render('template.html', sandboxed: true) }}
{{ render('template.html', _context, sandboxed: true, ignore_missing: true) }}
{{ render('template.html', sandboxed: true, ignore_missing: true) }}
{{ render('template.html', _context, sandboxed: true, ignore_missing: true) }}
{{ render('template.html', _context, sandboxed: true, ignore_missing: true) }}
{{ render('template.html', sandboxed: true, ignore_missing: true) }}

{{ render('template.html', _context) }}
{{ render('template.html', _context, sandboxed: true) }}
{{ render('template.html', _context, sandboxed: true, ignore_missing: true) }}
{{ render('template.html', _context, ignore_missing: true) }}
{{ render('template.html', _context, ignore_missing: true, sandboxed: true) }}

{{ render('template.html') }}
{{ render('template.html', sandboxed: true) }}
{{ render('template.html', sandboxed: true, ignore_missing: true) }}
{{ render('template.html', ignore_missing: true) }}
{{ render('template.html', ignore_missing: true, sandboxed: true) }}

@smnandre
Copy link
Contributor

smnandre commented Nov 9, 2024

And what should be the test i think to merge context with args

Arguments

{# from #}
{{ include('template.html', foobar }}
{{ include('template.html', {foo: bar}) }}
{{ include('template.html', {foobar} }}

{# to #}
{{ render('template.html', {..._context, ...foobar}) }}
{{ render('template.html', {..._context, foo: bar}) }}
{{ render('template.html', {..._context, foobar: foobar}) }}

Variable with_context

{# from #}
{{ include('template.html', foobar, with_context: var_name) }}
{{ include('template.html', {foo: bar}, with_context: var_name) }}
{{ include('template.html', {foobar}, with_context: var_name) }}

{# to #}
{{ render('template.html', {...(var_name ? _context : []), ...foobar}) }}
{{ render('template.html', {...(var_name ? _context : []), foo: bar}) }}
{{ render('template.html', {...(var_name ? _context : []), foobar: foobar}) }}

(need other opinions here... but these syntaxes may cover most of the needs)

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