Support unnamed function arguments to compiler plugins in Smarty 5#1088
Support unnamed function arguments to compiler plugins in Smarty 5#1088gkreitz wants to merge 3 commits intosmarty-php:masterfrom
Conversation
| // shorthand attribute ? | ||
| } elseif (isset($this->shorttag_order[$key])) { | ||
| $_indexed_attr[$this->shorttag_order[$key]] = $mixed; | ||
| } elseif (!is_null($this->getShorthandOrder($key))) { |
There was a problem hiding this comment.
I think the type of $key is always int here, based on the fact that it looks like $attributes is a list<mixed> from the usage, but I do not know enough about the code base to know. If my assumption is incorrect, the type of the argument to getShorthandOrder needs to be changed to avoid crashing.
There was a problem hiding this comment.
Well, all the unit tests pass and I haven't found any references to shorttag_order in the source that suggest other keys. But as you noted, it only looks that way and is not guaranteed. Just to be safe, I'd prefer to allow a mixed type for the key param to getShorthandOrder.
There was a problem hiding this comment.
Sorry for the slow response. I removed the php type annotations to be on the safe side.
From usage, it very much looks like the type will always be int, but on the off chance there is some code path where this gets called with something else, type was widened to mixed for now, per disussion in smarty-php#1088
gkreitz
force-pushed
the
1085_unnamed_compiler_arguments
branch
from
9bb3447 to
91dd198
Compare
From usage, it very much looks like the type will always be int, but on the off chance there is some code path where this gets called with something else, type was widened to mixed for now, per disussion in smarty-php#1088
gkreitz
force-pushed
the
1085_unnamed_compiler_arguments
branch
from
91dd198 to
0d24f1d
Compare
|
Gentle nudge to merge this if possible? |
3 participants
Fixes the regression in Smarty 5 where a compiler function using unnamed arguments threw a "Too many shorthand
attributes" error, and adds unit test coverage.
Fixes #1085