Skip to content

Fix GH-10489: run-tests.php does not escape path when building cmd - #10560

Merged
Girgias merged 2 commits into
php:masterfrom
ndossche:fix-10489
Feb 25, 2023
Merged

Fix GH-10489: run-tests.php does not escape path when building cmd #10560
Girgias merged 2 commits into
php:masterfrom
ndossche:fix-10489

Conversation

@ndossche

@ndosschendossche commented Feb 10, 2023•
edited
Loading

Copy link
Copy Markdown
Member

FixesGH-10489

Multiple tests had to be changed to escape the arguments in shell
commands. Some tests are skipped because they behave differently with
spaces in the path versus without. One notable example of this is the
hashbang test which does not work because spaces in hashbangs paths are
not supported in Linux.

mvorisek
mvorisek reviewed Feb 10, 2023
View reviewed changes
Comment threadtests/basic/bug71273.phpt Outdated
mvorisek
mvorisek reviewed Feb 10, 2023
View reviewed changes
Comment threadext/standard/tests/file/bug22414.phpt Outdated
mvorisek
mvorisek reviewed Feb 10, 2023
View reviewed changes
Comment threadext/standard/tests/file/popen_pclose_basic.phpt Outdated
mvorisek
mvorisek reviewed Feb 10, 2023
View reviewed changes
Comment threadrun-tests.php Outdated
@ndossche
ndosscheforce-pushed the fix-10489 branch 3 times, most recently from 7a0635a to 8cd9090CompareFebruary 11, 2023 13:57
@ndossche
ndossche marked this pull request as ready for review February 11, 2023 16:16
Girgias
Girgias reviewed Feb 13, 2023
View reviewed changes
Comment threadext/simplexml/tests/bug79971_1.phpt Outdated
Comment threadext/com_dotnet/tests/bug77578.phpt Outdated
Comment threadext/mbstring/tests/gh7902.phpt Outdated
@ndossche
ndosscheforce-pushed the fix-10489 branch 2 times, most recently from d77cb70 to cbc10c6CompareFebruary 13, 2023 20:41
@ndossche

ndossche commented Feb 13, 2023

Copy link
Copy Markdown
MemberAuthor

I applied @Girgias 's feedback and everything's green :).

mvorisek
mvorisek reviewed Feb 14, 2023
View reviewed changes
Comment threadext/standard/tests/streams/bug46024.phpt Outdated
@ndossche
ndosscheforce-pushed the fix-10489 branch 2 times, most recently from c25e501 to 8dae878CompareFebruary 15, 2023 08:40
mvorisek
mvorisek reviewed Feb 18, 2023
View reviewed changes
Comment threadrun-tests.php
@mvorisekmvorisek mentioned this pull request Feb 19, 2023
Merged
@Girgias

Girgias commented Feb 21, 2023

Copy link
Copy Markdown
Member

Can you please rebase this onto master, as trying to merge this upwards is a massive hassle and I don't think it's really worth it to fix in release versions...

@ndossche
ndossche changed the base branch from PHP-8.1 to masterFebruary 23, 2023 20:51
@ndossche

ndossche commented Feb 23, 2023

Copy link
Copy Markdown
MemberAuthor

Finally rebased this on master.

@Girgias

Girgias commented Feb 24, 2023

Copy link
Copy Markdown
Member

Windows seems to be failing for reasons

@ndossche

ndossche commented Feb 24, 2023

Copy link
Copy Markdown
MemberAuthor

Is it possible to retrigger Windows alone, or should I just rerun all of CI by doing rebase+force push?

@Girgias

Girgias commented Feb 25, 2023

Copy link
Copy Markdown
Member

Is it possible to retrigger Windows alone, or should I just rerun all of CI by doing rebase+force push?

I don't think I can retrigger appveyor sadly :/

ndosscheand others added 2 commits February 25, 2023 13:51
@ndossche
FixphpGH-10489: run-tests.php does not escape path when building cmd
4a3a2d4
Multiple tests had to be changed to escape the arguments in shell
commands. Some tests are skipped because they behave differently with
spaces in the path versus without. One notable example of this is the
hashbang test which does not work because spaces in hashbangs paths are
not supported in Linux.
@mvorisek@ndossche
honor cmd quotes on Windows in run-tests.php
f496f16
@Girgias
Girgias merged commit dcc3255 into php:masterFeb 25, 2023
@Girgias

Girgias commented Feb 25, 2023

Copy link
Copy Markdown
Member

Thank you!

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GirgiasGirgiasGirgias approved these changes
+1 more reviewer
@mvorisekmvorisekmvorisek left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Extension: com_dotnetExtension: mbstringExtension: simplexmlExtension: standardSAPI: cgiSAPI: cliSAPI: phpdbg

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

run-tests.php does not escape path when building cmd

3 participants

@ndossche@Girgias@mvorisek