8000 [Process] (Unwanted?) BC break with strict typing · Issue #28609 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Process] (Unwanted?) BC break with strict typing #28609

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
soullivaneuh opened this issue Sep 26, 2018 · 3 comments
Closed

[Process] (Unwanted?) BC break with strict typing #28609

soullivaneuh opened this issue Sep 26, 2018 · 3 comments
Labels
Bug Process Status: Needs Review

Comments

@soullivaneuh
Copy link
Contributor

Symfony version(s) affected: 4.1.4

Description

If you create a process with an array command-line containing null variable, it will fail:

Fatal error: Uncaught TypeError: Argument 1 passed to Symfony\Component\Process\Process::escapeArgument() must be of the type string, null given in /code/vendor/symfony/process/Process.php:1533
Stack trace:
#0 [internal function]: Symfony\Component\Process\Process->escapeArgument(NULL)
#1 /code/vendor/symfony/process/Process.php(262): array_map(Array, Array)
#2 /code/vendor/symfony/process/Process.php(200): Symfony\Component\Process\Process->start(NULL, Array)
#3 /code/test.php(13): Symfony\Component\Process\Process->run()
#4 {main}

How to reproduce

<?php

declare(strict_types=1);

require_once __DIR__.'/src/autoload.php';

$process = new \Symfony\Component\Process\Process([
    'ls -la',
    'test',
    null,
    'something',
]);
$process->run();

Possible Solution

I know it's a new major, but I found nothing about this issue on the upgrade guide. Plus, the error is not very explicit.

Two possibility:

  1. Cast all null parameter to string before using escapeArgument to have the same behavior as Symfony 3.
  2. Complete the upgrade guide and prevent the error by a more explicit one.
@soullivaneuh
Copy link
Contributor Author

BTW, you are still casting to string here to check empty arguments:

symfony/src/Symfony/Component/Process/Process.php

Lines 1575 to 1577 in 3cd411a

if ('' === $argument = (string) $argument) {
return '""';
}

But this is now not efficient with the strict typing, confirming a bug to me.

@stof
Copy link
Member
stof commented Sep 26, 2018

Well, I'm wondering whether null was ever meant to be supported in the array syntax of the Process class, even in 3.x

@soullivaneuh
Copy link
Contributor Author

@stof I don't know, but it was. BTW the current string casting inside the method seems to show us it is.

@vudaltsov vudaltsov mentioned this issue Oct 14, 2018
Merged
nicolas-grekas added a commit that referenced this issue Oct 14, 2018
@nicolas-grekas
bug #28863 [Process] Allow to pass non-string arguments to Process (v…
d010eb3 
…udaltsov)

This PR was merged into the 4.1 branch.

Discussion
----------

[Process] Allow to pass non-string arguments to Process

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #28609
| License       | MIT
| Doc PR        |

Sometimes you might pass integers, floats, nulls as command arguments to the Process constructor.
Before 4.0 and #24722 that worked fine. Now it throws because of an invalid type.

I think we can drop the type hint here safely and stringify the value implicitly in the method. That will be a little more convenient.

Commits
-------

acf8b83 Remove Process::escapeArgument argument type hint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Process Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@nicolas-grekas @stof @soullivaneuh @xabbuh @carsonbot
0