Add Rule IncludeFunction #176
Conversation
Now defined the same way in @symfony RuleSet (https://cs.symfony.com/doc/rules/import/ordered_imports.html) ``` @symfony with config: ['imports_order' => ['class', 'function', 'const'], 'sort_algorithm' => 'alpha'] ``` ```diff - 'ordered_imports' => [ - 'sort_algorithm' => 'alpha', - 'imports_order' => ['class', 'function', 'const'], - ], ```
Codecov ReportAll modified and coverable lines are covered by tests ✅
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #176 +/- ##
===========================================
Coverage 100.00% 100.00%
- Complexity 566 586 +20
===========================================
Files 51 53 +2
Lines 1741 1805 +64
===========================================
+ Hits 1741 1805 +64 ☔ View full report in Codecov by Sentry. |
| if ($withoutContext) { | ||
| $endInclude = ', with_context = false'.$endInclude; | ||
| } | ||
| if ($ignoreMissing) { | ||
| $endInclude = ', ignore_missing = true'.$endInclude; | ||
| } |
There was a problem hiding this comment.
I'm not sure about named parameters. I didn't see them often used in twig.
So I wonder if we shouldn't try to have
include('template.html' , vars , false)
rather than
include('template.html' , vars , with_context = false)
and same for others options.
There was a problem hiding this comment.
To be honest I have absolutely no idea wich of those two args (context / missing) is the first one / second one ..
Tried to follow "blindly" the doc : https://twig.symfony.com/doc/3.x/functions/include.html
I remove them and it can be the goal of another rule i guess later
There was a problem hiding this comment.
Oh well.. we generate "include(..., false, true)" if there was only "ignore_missing" ? :|
function twig_include(Environment $env, $context, $template, $variables = [], $withContext = true, $ignoreMissing = false, $sandboxed = false)
{There was a problem hiding this comment.
Yes, I had in mind to generate
Include ($template, [], false, true)
If the only option passed was "ignore missing".
I agree that "false" is unnecessary passed but I feel like people are not used to named parameter yet.
tests/Rules/Function/IncludeFunction/IncludeFunctionRuleTest.fixed.twig
Outdated
Show resolved
Hide resolved
Co-authored-by: Vincent Langlet <VincentLanglet@users.noreply.github.com>
| } | ||
| $endInclude = ') '.str_replace('%}', '}}', $tokens[$closingTag]->getValue()); | ||
| if ($ignoreMissing) { | ||
| $endInclude = sprintf(', %s', $ignoreMissing ? 'true' : 'false').$endInclude; |
There was a problem hiding this comment.
The ternary condition here is always true
|
Thanks ! |
| @@ -0,0 +1,16 @@ | |||
| {% include 'template.html' %} | |||
| {% include ['template_a.html', 'template_b.html'] ignore missing with {'foo': 'bar'} only %} | |||
| {% include 'template.html' only %} | |||
There was a problem hiding this comment.
@smnandre This is fixed in {{ include( 'template.html' , false) }}.
I think it's wrong, and should be {{ include( 'template.html', [], false) }}
Not sure if it's easy without named parameters...
There was a problem hiding this comment.
Damn really sorry to let so many mistakes :(
|
Waw nice one @smnandre and @VincentLanglet 👌🏼 |
Attempt to implement #140
Open to any feedback :)