[TwigBridge] lint all templates from configured Twig paths if no argument was provided #33446
Conversation
|
Thank you @yceruto. |
…aths if no argument was provided (yceruto) This PR was merged into the 4.4 branch. Discussion ---------- [TwigBridge] lint all templates from configured Twig paths if no argument was provided | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #33443 | License | MIT | Doc PR | - All details in PR title and related issue #33443. Commits ------- baddf1d lint all templates from configured Twig paths if no argument was provided
|
There's a conflict here: when no argument was provided, the command also reads from STDIN. This creates an ambiguous situation - maybe one did pipe nothing but doesn't expect the default template dir to be linted. I'd suggest resolving the ambiguity by reading from STDIN only when explicitly asked for. Passing |
|
I think we should revert this and trigger the deprecation instead... |
|
Maybe resolved by php/php-src#4684 ? Not sure also if reverting is the solution... because before these changes this statement |
|
Ok cool. Thanks for having a look. |
|
How is this code actually supposed to work? Under what circumstances do you want to use the stdin input and when not? Should it be used only with |
|
Test is still hanging despite the change: https://dev.azure.com/phpazuredevops/PHP/_build/results?buildId=2477 |
|
Unfortunately I'm not able to reproduce this issue locally. I've tried various variations of what to attach to stdin, but nothing produces the hang we see on azure. Possibly this just needs a |
|
tty group added in #33490 |
…olas-grekas) This PR was merged into the 4.4 branch. Discussion ---------- [Bridge/Twig] use tty group on testLintDefaultPaths | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #33446 (comment) | License | MIT | Doc PR | - Note that I still think we should deprecate reading from STDIN when not explicitly asked for, as explained in #33446 (comment) Commits ------- 3c59bb5 [Bridge/Twig] use tty group on testLintDefaultPaths
…ds (yceruto) This PR was merged into the 4.4 branch. Discussion ---------- Deprecated not passing dash symbol (-) to STDIN commands | Q | A | ------------- | --- | Branch? | 4.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #33446 (comment) | License | MIT | Doc PR | - Follow-up #33446 > There's a conflict here: when no argument was provided, the command also reads from STDIN. So now, it reads from STDIN, and if there is nothing there, reads from the default template. This has been caught in php/php-src#4672 > This creates an ambiguous situation - maybe one did pipe nothing but doesn't expect the default template dir to be linted. > I'd suggest resolving the ambiguity by reading from STDIN only when explicitly asked for. Passing - as argument could the way. And we could trigger a deprecation for now. For consistency, the other 2 lint commands (`lint:yaml` and `lint:xliff`) have been touched as well. The plan for 5.0 is read from the STDIN only when `-` is given. /cc @nicolas-grekas Commits ------- 586f299 deprecated not passing dash symbol (-) to STDIN commands
All details in PR title and related issue #33443.