Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] [Command] Clean bundle directory, fixes #23177#23195
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| } | ||
| } | ||
| if (is_dir($targetDir)) { | ||
| array_push($validAssetDir,$targetDir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
use$validAssetDir[] = $targetDir instead.
| $this->hardCopy($originDir,$targetDir); | ||
| } | ||
| } | ||
| if (is_dir($targetDir)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
instead of adding this condition, move this inside the previousif. There are 2 reasons for this:
- the variable is not assigned if the previous condition was false, which means it will either reuse the value from the previous iteration or fail with an undefined variable notice. Both are bad behaviors
- if we decided to install some assets for the bundle (which is what the previous condition is about), we know that the director us valid
| } | ||
| } | ||
| // Check in $bundlesDir, if all links/folder still have an existing Bundle | ||
| if ($dir =opendir($bundlesDir)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
use aDirectoryIterator. The code will be simpler:
- no need to skip
.and..(you can configure the iterator to skip them for you) - no need to close the directory at the end (btw, you don't properly close it if
$filesystem->removethrows an exception) - you can just do a
foreach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
opendir() 😆 it's at least 10 years ago since a last used that function (you make me feel old 😢😉).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
😭
ghost left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Thx@stof ,
i have used FilesystemIterator which is skipping dots by default
| } | ||
| } | ||
| // Check in $bundlesDir, if all links/folder still have an existing Bundle | ||
| foreach (new \FilesystemIterator($bundlesDir)as$file) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
FYI it's actually a dir not a file 😉 other parts looks good 👍
| $output->writeln('Installing assets as <comment>hard copies</comment>.'); | ||
| } | ||
| $validAssetDir =array(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The variable name should be a plural. It is not a directory, but a list of directories.
| $validAssetDirs[] =$targetDir; | ||
| } | ||
| } | ||
| // Check in $bundlesDir, if all links/folder still have an existing Bundle |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This comment explains "how it does" something, but I'd recommend to explain "what it does" instead. Something like this:
// remove the assets of the bundles that no longer existThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
ok@javiereguiluz 👍
fabpot commentedJun 16, 2017
Thank you @NicolasPion. |
…23177 (NicolasPion)This PR was squashed before being merged into the 2.7 branch (closes#23195).Discussion----------[FrameworkBundle] [Command] Clean bundle directory,fixes#23177| Q | A| ------------- | ---| Branch? | 2.7 <!-- see comment below -->| Bug fix? | yes| New feature? | no <!-- don't forget updating src/**/CHANGELOG.md files -->| BC breaks? | no| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->| Tests pass? | no| Fixed tickets |#23177| License | MITThis PRfix#23177when running an assets:install, it will remove directorys who do not have anymore a valid BundleCommits-------180f178 [FrameworkBundle] [Command] Clean bundle directory,fixes#23177
This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle] Do not remove files from assets dir| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -The patch introduced in#23195 removes files from `web/bundles` (eg. `.gitignore`) which is unintentional I think.Commits-------6ed9c8d [FrameworkBundle] Do not remove files from assets dir
This PRfix#23177
when running an assets:install, it will remove directorys who do not have anymore a valid Bundle