Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Process] Allow to pass null arguments to Process#28863

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.

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

Already on GitHub?Sign in to your account

Merged
nicolas-grekas merged 1 commit intosymfony:4.1fromvudaltsov:patch-2
Oct 14, 2018

Conversation

@vudaltsov
Copy link
Contributor

@vudaltsovvudaltsov commentedOct 14, 2018•
edited
Loading

QA
Branch?4.1
Bug fix?yes
New feature?no
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#28609
LicenseMIT
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.

@nicolas-grekas
Copy link
Member

Should target 3.4 is suppose. Could you please add a test case also?

@vudaltsov
Copy link
ContributorAuthor

vudaltsov commentedOct 14, 2018•
edited
Loading

@nicolas-grekas , typehints were added in 4.0 with php 7.0. In 3.4 everything should work fine (seehttps://github.com/symfony/symfony/blob/3.4/src/Symfony/Component/Process/Process.php#L1714).

nicolas-grekas reacted with thumbs up emoji

@vudaltsovvudaltsov changed the base branch frommaster to4.1October 14, 2018 17:13
@nicolas-grekasnicolas-grekas added this to the4.1 milestoneOct 14, 2018
@nicolas-grekas
Copy link
Member

OK, so maybe only the test case should target 3.4 then?

@vudaltsov
Copy link
ContributorAuthor

Rebased onto 4.1, since 4.0 is for security fixes only.

Ok, I will add another PR with a test case for 3.4 soon.

@vudaltsov
Copy link
ContributorAuthor

@nicolas-grekas , a test case is ready in#28864

nicolas-grekas added a commit that referenced this pull requestOct 14, 2018
…nts (vudaltsov)This PR was merged into the 3.4 branch.Discussion----------[Process] A test case for stringifying of Process arguments| Q             | A| ------------- | ---| Branch?       | 3.4| Bug fix?      | no| New feature?  | no| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets | for#28863| License       | MIT| Doc PR        |Checks that non-string arguments passed to Process constructor are typecasted to stringCommits-------0d54342 Add a test case for stringifying of Process arguments
@vudaltsov
Copy link
ContributorAuthor

Ready!

@vudaltsov
Copy link
ContributorAuthor

@nicolas-grekas , addressed your comment

@nicolas-grekasnicolas-grekas changed the title[Process] Allow to pass non-string arguments to Process[Process] Allow to pass null arguments to ProcessOct 14, 2018
@nicolas-grekas
Copy link
Member

Thank you@vudaltsov.

vudaltsov reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commitacf8b83 intosymfony:4.1Oct 14, 2018
nicolas-grekas added a commit that referenced this pull requestOct 14, 2018
…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
@soullivaneuh
Copy link
Contributor

Thank you@vudaltsov! :-)

vudaltsov reacted with hooray emoji

@vudaltsovvudaltsov deleted the patch-2 branchOctober 15, 2018 08:55
@vudaltsov
Copy link
ContributorAuthor

@nicolas-grekas , the point of my PR was to allow to pass ints and floats as well... Why did you remove that?

@vudaltsov
Copy link
ContributorAuthor

vudaltsov commentedOct 15, 2018•
edited
Loading

I have two arguments:

  1. that was possible before. so it's a kind of BC break.
  2. it's easier to donew Process(['sync:user', $id]) thannew Process(['sync:user', (string) $id]). A bit more developer friendly

@nicolas-grekas
Copy link
Member

I didn't remove that: this is PHP language, where types are cast automatically for you when needed.

@vudaltsov
Copy link
ContributorAuthor

Am I right that this is because we don't havedeclare(strict_types=1);?

@nicolas-grekas
Copy link
Member

That's correct.

@fabpotfabpot mentioned this pull requestNov 3, 2018
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas left review comments

Assignees

No one assigned

Projects

None yet

Milestone

4.1

Development

Successfully merging this pull request may close these issues.

4 participants

@vudaltsov@nicolas-grekas@soullivaneuh@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp