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

Already on GitHub? Sign in to your account

[Filesystem] Consistence and enhancements for Filesystem #4330

Merged
merged 1 commit into from Jun 19, 2012

Conversation

Projects
None yet
7 participants
Member

romainneutron commented May 18, 2012

Bug fix: no
Feature addition: yes
Backwards compatibility break: yes
Symfony2 tests pass: yes
Fixes the following tickets: None
License of the code: MIT

This PR adds features and introduce a backward compatibility break.

features :

  • whenever an action fails, a \RuntimeException is thrown
  • add access to the second and third arguments of touch function
  • add a recursive option for chmod
  • add a chown method
  • add a chgrp method

The backward compatibility break happens in the mkdir method : Before this PR, a boolean is returned ; true if all directories were created, false otherwise.
It now returns nothing.

This pull request passes (merged 83cdd622 into 1e15f21).

Owner

fabpot commented May 20, 2012

To be consistent, we should throw exception whenever some operation fails.

Member

romainneutron commented May 20, 2012

I fix the consistency ; mkdir now throws an exception if a directory creation fails.
This introduce a BC break, see PR message which has been updated with all features and BC break.

Added chgrp and chown methods
Add options for touch
Add recursive option for chmod

This pull request passes (merged a4d1eeb8 into 1407f11).

@stloyd stloyd and 1 other commented on an outdated diff May 22, 2012

src/Symfony/Component/Filesystem/Filesystem.php
{
+ if (is_null($time)) {
@stloyd

stloyd May 22, 2012

Contributor

You need to use: null === $time.

@stloyd stloyd and 1 other commented on an outdated diff May 22, 2012

src/Symfony/Component/Filesystem/Filesystem.php
foreach ($this->toIterator($files) as $file) {
- touch($file);
+ if(false === @touch($file, $time, $atime)) {
@stloyd

stloyd May 22, 2012

Contributor

Missing space after if.

@stloyd stloyd and 1 other commented on an outdated diff May 22, 2012

src/Symfony/Component/Filesystem/Filesystem.php
@@ -95,9 +108,13 @@ public function remove($files)
if (is_dir($file) && !is_link($file)) {
$this->remove(new \FilesystemIterator($file));
- rmdir($file);
+ if (false === rmdir($file)) {
@stloyd

stloyd May 22, 2012

Contributor

You should also add @ here and below, to prevent errors throws when permissions are insufficient.

@stloyd stloyd and 1 other commented on an outdated diff May 22, 2012

src/Symfony/Component/Filesystem/Filesystem.php
{
foreach ($this->toIterator($files) as $file) {
- @chmod($file, $mode & ~$umask);
+ if ($recursive && is_dir($file) && !is_link($file)) {
+ $this->chmod(new \FilesystemIterator($file), $mode, $umask, true);
+ }
+ if (!chmod($file, $mode & ~$umask)) {
@stloyd

stloyd May 22, 2012

Contributor

You should not remove @ here. Also you should be consistent IMO and use comparsion: false === @chmod()

@romainneutron

romainneutron May 22, 2012

Member

chmod sometimes returns null, for example : this returns null :

var_dump(@chmod(__FILE__, 'hello'));
@stloyd

stloyd May 22, 2012

Contributor

IMO this is not valid use case as you never can change perms of file you execute actually. Is there any case when it not return false on failture ? (docs + comments don't mention any)

@romainneutron

romainneutron May 22, 2012

Member

I mean, chmod returns null if the mod is not valid, it returns false if the mod fails :

This example dumps null

<?php
$file = __DIR__ . '/test';
touch($file);
var_dump(@chmod($file, 'hello'));

and this example dumps false

<?php
//$file is owned by root, I can't chmod it
$file = __DIR__ . '/owned_by_root';
var_dump(@chmod($file, 0777));
@romainneutron

romainneutron May 22, 2012

Member

I now check the result of chmod against true, tell me what you think about it

@stloyd stloyd and 1 other commented on an outdated diff May 22, 2012

src/Symfony/Component/Filesystem/Filesystem.php
+ * Change the owner of an array of files or directories
+ *
+ * @param string|array|\Traversable $files A filename, an array of files, or a \Traversable instance to change owner
+ * @param string $user The new owner user name
+ * @param boolean $recursive Wheter change the owner recursively or not
+ *
+ * @throws \RuntimeException When the change fail
+ */
+ public function chown($files, $user, $recursive = false)
+ {
+ foreach ($this->toIterator($files) as $file) {
+ if ($recursive && is_dir($file) && !is_link($file)) {
+ $this->chown(new \FilesystemIterator($file), $user, true);
+ }
+ if (is_link($file) && function_exists('lchown')) {
+ if(!@lchown($file, $user)) {
@stloyd

stloyd May 22, 2012

Contributor

Missing space after if.

@stloyd stloyd and 1 other commented on an outdated diff May 22, 2012

src/Symfony/Component/Filesystem/Filesystem.php
+ * Change the group of an array of files or directories
+ *
+ * @param string|array|\Traversable $files A filename, an array of files, or a \Traversable instance to change group
+ * @param string $group The group name
+ * @param boolean $recursive Wheter change the group recursively or not
+ *
+ * @throws \RuntimeException When the change fail
+ */
+ public function chgrp($files, $group, $recursive = false)
+ {
+ foreach ($this->toIterator($files) as $file) {
+ if ($recursive && is_dir($file) && !is_link($file)) {
+ $this->chgrp(new \FilesystemIterator($file), $group, true);
+ }
+ if (is_link($file) && function_exists('lchgrp')) {
+ if(!@lchgrp($file, $group)) {
@stloyd

stloyd May 22, 2012

Contributor

Missing space after if.

@stloyd stloyd commented on the diff May 22, 2012

src/Symfony/Component/Filesystem/Filesystem.php
}
/**
- * Creates empty files.
+ * Sets access and modification time of file.
*
* @param string|array|\Traversable $files A filename, an array of files, or a \Traversable instance to create
@stloyd

stloyd May 22, 2012

Contributor

Missing info about new params.

@stloyd stloyd and 1 other commented on an outdated diff May 22, 2012

src/Symfony/Component/Filesystem/Filesystem.php
@@ -108,11 +125,72 @@ public function remove($files)
* @param string|array|\Traversable $files A filename, an array of files, or a \Traversable instance to change mode
* @param integer $mode The new mode (octal)
* @param integer $umask The mode mask (octal)
@stloyd

stloyd May 22, 2012

Contributor

Missing info about new param.

@stloyd stloyd and 1 other commented on an outdated diff May 22, 2012

src/Symfony/Component/Filesystem/Filesystem.php
+ foreach ($this->toIterator($files) as $file) {
+ if ($recursive && is_dir($file) && !is_link($file)) {
+ $this->chmod(new \FilesystemIterator($file), $mode, $umask, true);
+ }
+ if (!@chmod($file, $mode & ~$umask)) {
+ throw new \RuntimeException(sprintf('Failed to chmod file %s', $file));
+ }
+ }
+ }
+
+ /**
+ * Change the owner of an array of files or directories
+ *
+ * @param string|array|\Traversable $files A filename, an array of files, or a \Traversable instance to change owner
+ * @param string $user The new owner user name
+ * @param boolean $recursive Wheter change the owner recursively or not
@stloyd

stloyd May 22, 2012

Contributor

Should be Boolean. Same below.

This pull request passes (merged 7e14b6bd into 517ae43).

This pull request passes (merged 71852653 into 517ae43).

This pull request passes (merged 7645bad3 into 517ae43).

This pull request fails (merged b049d5b1 into 517ae43).

This pull request fails (merged 34903466 into 517ae43).

This pull request passes (merged b1d1eb2e into adf07f1).

This pull request passes (merged 42015ffa into adf07f1).

Member

romainneutron commented Jun 1, 2012

Any news about this PR ?

@pborreli pborreli commented on the diff Jun 8, 2012

src/Symfony/Component/Filesystem/Filesystem.php
@@ -54,27 +58,36 @@ public function copy($originFile, $targetFile, $override = false)
*/
@pborreli

pborreli Jun 8, 2012

Contributor

you should remove the line :

     * @return Boolean true if the directory has been created, false otherwise
@romainneutron

romainneutron Jun 8, 2012

Member

done, and exception doc added

Contributor

stloyd commented Jun 8, 2012

@romainneutron You need to squash your commits, and add more proper message in squashed commit i.e.:

[Filesystem] Added few new behaviors:

  • whenever an action fails, a RuntimeException is thrown
  • add access to the second and third arguments of touch() function
  • add a recursive option for chmod()
  • add a chown() method
  • add a chgrp() method

BC break: mkdir() function throw exception in case of failture instead of returning Boolean value.

Member

romainneutron commented Jun 8, 2012

@stloyd squash done !

This pull request passes (merged 8f55ddb6 into f8e68e5).

This pull request passes (merged 880312b6 into f8e68e5).

Member

romainneutron commented Jun 9, 2012

I've added documentation to the Filesystem component : symfony/symfony-docs#1439

This pull request passes (merged 5647ad41 into f8a09db).

Contributor

stloyd commented Jun 13, 2012

@romainneutron You probably need to rebase your code as some changes were merge into master for Filesystem.

Member

romainneutron commented Jun 13, 2012

@stloyd rebase OK !

by the way, do you have any idea when/if this PR will be merged ?

This pull request passes (merged c8b86c68 into c07e916).

Owner

fabpot commented Jun 16, 2012

You need to add a note about the BC breaks in the CHANGELOG file.

Owner

fabpot commented Jun 16, 2012

Also, instead of using \RuntimeException, I would create a custom exception like we have done in other components (an interface + a RuntimeException that implements the interface and extends \RuntimeException). The exception name can be something like IOException.

@weaverryan weaverryan referenced this pull request in symfony/symfony-docs Jun 17, 2012

Merged

Add Filesystem component documentation #1439

@Burgov Burgov and 1 other commented on an outdated diff Jun 18, 2012

src/Symfony/Component/Filesystem/Filesystem.php
} else {
$ok = true;
}
}
if (!$ok) {
- symlink($originDir, $targetDir);
+ if (true !== symlink($originDir, $targetDir)) {
@Burgov

Burgov Jun 18, 2012

Contributor

Shouldn't an "@" be added here?

This pull request fails (merged 925a8234 into 0b8b76b).

Contributor

stloyd commented Jun 18, 2012

@fabpot Anything blocking merge of this PR ? (tests are failing because of issue in master, not releted to this PR)

Member

romainneutron commented Jun 18, 2012

@fabpot @stloyd the latest push was just a rebase push for PR 4577 (#4577)
I'm currently fixing the Exception and changelog things, I'll push very soon

Member

romainneutron commented Jun 18, 2012

@fabpot I've added the exception and the exception interface, add the changelog info

This pull request fails (merged 634d6fb9 into 0b8b76b).

Member

romainneutron commented Jun 18, 2012

As reported by @stloyd the PR is failing due to an issue in the master, I re-push and trig the PR build when this issue is solved

This pull request fails (merged 2f65945a into 0b8b76b).

@stloyd stloyd and 1 other commented on an outdated diff Jun 18, 2012

src/Symfony/Component/Filesystem/Filesystem.php
@@ -28,6 +28,8 @@ class Filesystem
* @param string $originFile The original filename
* @param string $targetFile The target filename
* @param array $override Whether to override an existing file or not
+ *
+ * @throws Exception\IOException When copy fails
@stloyd

stloyd Jun 18, 2012

Contributor

This is wrong, you need to add use Symfony\Component\Filesystem\Exception\IOException; and use only class name here and all way below.

@romainneutron

romainneutron Jun 18, 2012

Member

this has been fixed

This pull request fails (merged b1f8744 into 0b8b76b).

This pull request fails (merged 832fd355 into 0b8b76b).

@romainneutron romainneutron Merge pull request #1 from SamsonIT/FilesystemExceptions
[FileSystem] explain possible failure of symlink creation in windows
a20fc68

This pull request passes (merged a20fc68 into 0b8b76b).

@fabpot fabpot added a commit that referenced this pull request Jun 19, 2012

@fabpot fabpot merged branch romainneutron/FilesystemExceptions (PR #4330)
Commits
-------

a20fc68 Merge pull request #1 from SamsonIT/FilesystemExceptions
8eca661 [FileSystem] explains possible failure of symlink creation in windows
b1f8744 Add Changelog BC Break note
24eb396 [Filesystem] Added few new behaviors:

Discussion
----------

[Filesystem] Consistence and enhancements for Filesystem

Bug fix: no
Feature addition: yes
Backwards compatibility break: **yes**
Symfony2 tests pass: yes
Fixes the following tickets: None
License of the code: MIT

This PR adds features and introduce a backward compatibility break.

features :
- whenever an action fails, a \RuntimeException is thrown
- add access to the second and third arguments of ``touch`` function
- add a recursive option for chmod
- add a chown method
- add a chgrp method

The backward compatibility break happens in the mkdir method : Before this PR, a boolean is returned ; true if all directories were created, false otherwise.
It now returns nothing.

---------------------------------------------------------------------------

by travisbot at 2012-05-18T14:26:42Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1367000) (merged 83cdd622 into 1e15f21).

---------------------------------------------------------------------------

by fabpot at 2012-05-20T02:40:28Z

To be consistent, we should throw exception whenever some operation fails.

---------------------------------------------------------------------------

by romainneutron at 2012-05-20T21:10:23Z

I fix the consistency ; mkdir now throws an exception if a directory creation fails.
This introduce a BC break, see PR message which has been updated with all features and BC break.

Added chgrp and chown methods
Add options for touch
Add recursive option for chmod

---------------------------------------------------------------------------

by travisbot at 2012-05-20T21:11:47Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1383619) (merged a4d1eeb8 into 1407f11).

---------------------------------------------------------------------------

by travisbot at 2012-05-22T10:49:06Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1399027) (merged 7e14b6bd into 517ae43).

---------------------------------------------------------------------------

by travisbot at 2012-05-22T10:58:10Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1399083) (merged 71852653 into 517ae43).

---------------------------------------------------------------------------

by travisbot at 2012-05-22T11:18:44Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1399194) (merged 7645bad3 into 517ae43).

---------------------------------------------------------------------------

by travisbot at 2012-05-23T18:21:47Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1414091) (merged b049d5b1 into 517ae43).

---------------------------------------------------------------------------

by travisbot at 2012-05-23T18:26:19Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1414123) (merged 34903466 into 517ae43).

---------------------------------------------------------------------------

by travisbot at 2012-05-29T16:07:26Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1467173) (merged b1d1eb2e into adf07f1).

---------------------------------------------------------------------------

by travisbot at 2012-05-29T16:19:38Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1467261) (merged 42015ffa into adf07f1).

---------------------------------------------------------------------------

by romainneutron at 2012-06-01T14:30:45Z

Any news about this PR ?

---------------------------------------------------------------------------

by stloyd at 2012-06-08T09:57:39Z

@romainneutron You need to [squash](http://www.silverwareconsulting.com/index.cfm/2010/6/6/Using-Git-Rebase-to-Squash-Commits) your commits, and add more proper message in squashed commit i.e.:

> [Filesystem]  Added few new behaviors:
* whenever an action fails, a `RuntimeException` is thrown
* add access to the second and third arguments of `touch()` function
* add a recursive option for `chmod()`
* add a `chown()` method
* add a `chgrp()` method

> BC break: `mkdir()` function throw exception in case of failture instead of returning Boolean value.

---------------------------------------------------------------------------

by romainneutron at 2012-06-08T10:59:55Z

@stloyd squash done !

---------------------------------------------------------------------------

by travisbot at 2012-06-08T11:26:20Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1565540) (merged 8f55ddb6 into f8e68e5).

---------------------------------------------------------------------------

by travisbot at 2012-06-08T11:41:45Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1566247) (merged 880312b6 into f8e68e5).

---------------------------------------------------------------------------

by romainneutron at 2012-06-09T11:42:24Z

I've added documentation to the Filesystem component : symfony/symfony-docs#1439

---------------------------------------------------------------------------

by travisbot at 2012-06-09T16:47:20Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1577754) (merged 5647ad41 into f8a09db).

---------------------------------------------------------------------------

by stloyd at 2012-06-13T14:47:31Z

@romainneutron You probably need to rebase your code as some changes were merge into master for `Filesystem`.

---------------------------------------------------------------------------

by romainneutron at 2012-06-13T15:17:31Z

@stloyd rebase OK !

by the way, do you have any idea when/if this PR will be merged ?

---------------------------------------------------------------------------

by travisbot at 2012-06-13T15:20:44Z

This pull request [passes](http://travis-ci.org/symfony/symfony/builds/1611591) (merged c8b86c68 into c07e916).

---------------------------------------------------------------------------

by fabpot at 2012-06-16T16:40:50Z

You need to add a note about the BC breaks in the CHANGELOG file.

---------------------------------------------------------------------------

by fabpot at 2012-06-16T16:43:20Z

Also, instead of using `\RuntimeException`, I would create a custom exception like we have done in other components (an interface + a RuntimeException that implements the interface and extends \RuntimeException). The exception name can be something like `IOException`.

---------------------------------------------------------------------------

by travisbot at 2012-06-18T10:11:20Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1645757) (merged 925a8234 into 0b8b76b).

---------------------------------------------------------------------------

by stloyd at 2012-06-18T10:14:52Z

@fabpot Anything blocking merge of this PR ? (tests are failing because of issue in master, not releted to this PR)

---------------------------------------------------------------------------

by romainneutron at 2012-06-18T10:29:20Z

@fabpot @stloyd the latest push was just a rebase push for PR 4577 (#4577)
I'm currently fixing the Exception and changelog things, I'll push very soon

---------------------------------------------------------------------------

by romainneutron at 2012-06-18T10:44:38Z

@fabpot I've added the exception and the exception interface, add the changelog info

---------------------------------------------------------------------------

by travisbot at 2012-06-18T10:53:34Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1645981) (merged 634d6fb9 into 0b8b76b).

---------------------------------------------------------------------------

by romainneutron at 2012-06-18T11:08:43Z

As reported by @stloyd the PR is failing due to an issue in the master, I re-push and trig the PR build when this issue is solved

---------------------------------------------------------------------------

by travisbot at 2012-06-18T11:16:58Z

This pull request [fails](http://travis-ci.org/symfony/symfony/builds/1646006) (merged 2f65945a into 0b8b76b).
55f682c

@fabpot fabpot merged commit a20fc68 into symfony:master Jun 19, 2012

Contributor

willdurand commented Jun 19, 2012

0 additions, 0 deletions? :o

Member

romainneutron commented Jun 19, 2012

@willdurand it seems to be a github bug

here are the commits

24eb396
b1f8744
8eca661

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