Remove redundant sprintf argument. #24613
Conversation
| @@ -53,6 +53,6 @@ protected function getVariableGetterWithStrictCheck($name) | |||
| return sprintf('(isset($context["%s"]) || array_key_exists("%s", $context) ? $context["%s"] : (function () { throw new Twig_Error_Runtime(\'Variable "%s" does not exist.\', 0, $this->getSourceContext()); })())', $name, $name, $name, $name); | |||
| } | |||
|
|
|||
| return sprintf('($context["%s"] ?? $this->getContext($context, "%s"))', $name, $name, $name); | |||
| return sprintf('($context["%s"] ?? $this->getContext($context, "%s"))', $name, $name); | |||
There was a problem hiding this comment.
%1$s? thus assign $name once here as well.
There was a problem hiding this comment.
Don't you find that notation (%1$s) difficult to understand? I always struggle with it.
There was a problem hiding this comment.
No strong opinion. Just to make things less error prone :) basically the two assignments must always stay the same, thus id prefer a single assignment.
There was a problem hiding this comment.
@ro0NL I thought about it, but decided not to change. But I agree that %1$s is better.
There was a problem hiding this comment.
I prefer %1$s. That's how sprintf() should be used to avoid duplications, and we already have some usages in the code base
|
This PR should be opened against 2.7 as it is wrong there as well. |
|
@Tobion there is different code in different branches: So in some branches there is php version checks, in others – not. May be merge this pr to branches w/o php version checks and create another one for other branches? |
|
@Aliance it's enough if you open a PR for 2.7. We then merge it upwards and will resolve conflicts. |
0b88aed to
c8012f0
Compare
|
@Tobion I rebased it on 2.7, also changed duplicated arguments to |
| @@ -437,7 +437,7 @@ private function initialize() | |||
| // corresponding elements are either descendants or have a matching HTML5 form attribute | |||
| $formId = Crawler::xpathLiteral($this->node->getAttribute('id')); | |||
|
|
|||
| $fieldNodes = $xpath->query(sprintf('descendant::input[@form=%s] | descendant::button[@form=%s] | descendant::textarea[@form=%s] | descendant::select[@form=%s] | //form[@id=%s]//input[not(@form)] | //form[@id=%s]//button[not(@form)] | //form[@id=%s]//textarea[not(@form)] | //form[@id=%s]//select[not(@form)]', $formId, $formId, $formId, $formId, $formId, $formId, $formId, $formId)); | |||
| $fieldNodes = $xpath->query(sprintf('descendant::input[@form=%s] | descendant::button[@form=%1$s] | descendant::textarea[@form=%1$s] | descendant::select[@form=%1$s] | //form[@id=%1$s]//input[not(@form)] | //form[@id=%1$s]//button[not(@form)] | //form[@id=%1$s]//textarea[not(@form)] | //form[@id=%1$s]//select[not(@form)]', $formId)); | |||
There was a problem hiding this comment.
lol :) definitely up for %1$s now.
|
Good catch, thanks @Aliance. |
This PR was merged into the 2.7 branch. Discussion ---------- Remove redundant sprintf argument. | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #22820 | License | MIT | Doc PR | – cc @fabpot @ogizanagi Commits ------- c8012f0 Remove redundant sprintf arguments.
cc @fabpot @ogizanagi