Skip to content

[Filesystem] chown and chgrp should also accept int as owner and group #35356

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
Merged
merged 1 commit into from
Jan 21, 2020
Merged

[Filesystem] chown and chgrp should also accept int as owner and group #35356

merged 1 commit into from
Jan 21, 2020

Conversation

Slamdunk
Copy link
Contributor

Q A
Branch? 5.0 for bug fixes
Bug fix? yes
New feature? no
Deprecations? no
Tickets
License MIT
Doc PR

Both chown and chgrp accept both string for username and int for user id.

The string typehint was only in the docblock till 4.4, so it was non-blocking.

@Slamdunk Slamdunk changed the title Add tests [Filesystem] chown and chgrp should also accept int as owner and group Jan 16, 2020
@Slamdunk
Copy link
Contributor Author

The are two fails: the Travis' one seems unrelated

immagine

The AppVeyor one I don't really know how to fix it:

immagine

Any help?

@nicolas-grekas
Copy link
Member

Why is this better than providing the integer as a string? I think string is good as a type hint.

@nicolas-grekas nicolas-grekas added this to the 5.0 milestone Jan 17, 2020
@Slamdunk
Copy link
Contributor Author

Because it doesn't work:

# date > file.txt

# ll
-rw-r--r-- 1 root root 31 gen 17 09:29 file.txt

# php -r 'var_dump(chown("file.txt", "1000"));'
PHP Warning:  chown(): Unable to find uid for 1000 in Command line code on line 1
bool(false)

# ll
-rw-r--r-- 1 root root 31 gen 17 09:29 file.txt

# php -r 'var_dump(chown("file.txt", 1000));'
bool(true)

# ll
-rw-r--r-- 1 tessarotto root 31 gen 17 09:29 file.txt

# php -v
PHP 7.3.13-1+ubuntu19.10.1+deb.sury.org+1 (cli) (built: Dec 18 2019 14:49:29) ( NTS )

@Slamdunk
Copy link
Contributor Author

Slamdunk commented Jan 17, 2020 •
edited
Loading

I've push the first commit with only the tests, so you can see them failing the first time: https://travis-ci.org/symfony/symfony/jobs/637855585#L3952

@nicolas-grekas
Copy link
Member

OK, thanks! Can you send a PR to 3.4 to update the docblock please?

@Slamdunk Slamdunk mentioned this pull request Jan 17, 2020
Merged
@Slamdunk
Copy link
Contributor Author

Done

@xabbuh
Copy link
Member

xabbuh commented Jan 17, 2020

Tests on AppVeyor are failing now. Can you please check how to fix them?

@Slamdunk
Copy link
Contributor Author

Honestly I don't get why FilesystemTest::testChgrpSymlinkById is run in the first place.
The method before, FilesystemTest::testChgrpSymlinkByName, gets correctly skipped.

How can I debug this without a Windows machine? Shrinking .appveyor.yml scope it's not so trivial for me :\

nicolas-grekas added a commit that referenced this pull request Jan 17, 2020
@nicolas-grekas
minor #35369 [Filesystem] chown and chgrp should also accept int as o…
f889c11 
…wner and group (3.4) (Slamdunk)

This PR was merged into the 3.4 branch.

Discussion
----------

[Filesystem] chown and chgrp should also accept int as owner and group (3.4)

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Tickets       |
| License       | MIT
| Doc PR        |

Reference: #35356 (comment)

Commits
-------

6b811e6 chown and chgrp should also accept int as owner and group
nicolas-grekas
nicolas-grekas reviewed Jan 20, 2020
View reviewed changes
src/Symfony/Component/Filesystem/Tests/FilesystemTest.php
@@ -669,6 +710,23 @@ public function testChgrpSymlink()
$this->assertSame($group, $this->getFileGroup($link));
}

public function testChgrpSymlinkById()
{
$this->markAsSkippedIfSymlinkIsMissing();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

symlink is not missing on appveyor, so this doesn't skip Windows.
use if ('\\' === \DIRECTORY_SEPARATOR) {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unable to understand the bug as is: I'll create a separate playground PR to debug it

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about calling markAsSkippedIfPosixIsMissing() instead? I think chgrp() doesn't work otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, that's what I've done now: https://github.com/symfony/symfony/pull/35356/files#diff-f912c7214d9bb13479ead974deabe7d3R126

@Slamdunk Slamdunk mentioned this pull request Jan 20, 2020
Closed
@nicolas-grekas
Copy link
Member

Thank you @Slamdunk.

nicolas-grekas added a commit that referenced this pull request Jan 21, 2020
@nicolas-grekas
bug #35356 [Filesystem] chown and chgrp should also accept int as own…
aae900a 
…er and group (Slamdunk)

This PR was squashed before being merged into the 5.0 branch.

Discussion
----------

[Filesystem] chown and chgrp should also accept int as owner and group

| Q             | A
| ------------- | ---
| Branch?       | 5.0 for bug fixes
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       |
| License       | MIT
| Doc PR        |

Both [chown](https://www.php.net/manual/en/function.chown.php) and [chgrp](https://www.php.net/manual/en/function.chgrp.php) accept both `string` for username and `int` for user id.

The string typehint was only in the docblock till 4.4, so it was non-blocking.

Commits
-------

eba5a0c [Filesystem] chown and chgrp should also accept int as owner and group
@nicolas-grekas nicolas-grekas merged commit eba5a0c into symfony:5.0 Jan 21, 2020
@Slamdunk Slamdunk deleted the chown_int branch January 21, 2020 08:52
@fabpot fabpot mentioned this pull request Jan 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xabbuh xabbuh xabbuh left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

Assignees
No one assigned
Labels
Bug Filesystem Status: Reviewed
Projects
None yet
Milestone
5.0
Development

Successfully merging this pull request may close these issues.
4 participants
@Slamdunk @nicolas-grekas @xabbuh @carsonbot