Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removed system calls and glob function in FileManager #14

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

alexandre-melard
Copy link

removed glob functions call, system function call and rsync call.
Replaced with symfony2 Finder and Filesystem classes

removed glob functions call, system function call and rsync call.
Replaced with symfony2 Finder and Filesystem classes

public function __construct($options)
{
if (!strlen(trim($options['file_base_path']))) {
Copy link
Author

Choose a reason for hiding this comment

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

I prefer to change the checking for conf parameter in constructor as it is always called before the other functions

@boutell
Copy link
Member

boutell commented Nov 14, 2012

Thanks for the pull request! However as written this change will break the syncing of content back to the permanent folder on a later edit of an existing object. Files that are removed from the collection in the editor need to be removed from the permanent folder. The syncFiles() method must do what it promises to do - if we ditch rsync then we must still provide the functionality of rsync.

This is an example of code in my project that would break with this change:

$fileUploader->syncFiles(
array('from_folder' => '/tmp/photos/' . $editId,
'to_folder' => '/photos/' . $posting->getId(),
'remove_from_folder' => true,
'create_to_folder' => true));

@alexandre-melard
Copy link
Author

OK, I will add that :)
What do you think of the use of Finder and Filesystem classes by the way?

On 14 November 2012 17:39, Tom Boutell [email protected] wrote:

Thanks for the pull request! However as written this change will break the
syncing of content back to the permanent folder on a later edit of an
existing object. Files that are removed from the collection in the editor
need to be removed from the permanent folder. The syncFiles() method must
do what it promises to do - if we ditch rsync then we must still provide
the functionality of rsync.

This is an example of code in my project that would break with this change:

$fileUploader->syncFiles(
array('from_folder' => '/tmp/photos/' . $editId,
'to_folder' => '/photos/' . $posting->getId(),
'remove_from_folder' => true,
'create_to_folder' => true));


Reply to this email directly or view it on GitHubhttps://github.com//pull/14#issuecomment-10373108.

@boutell
Copy link
Member

boutell commented Nov 14, 2012

There will almost surely be a performance hit compared to rsync, but I like
the idea of an option to do it this way. I'd like rsync to remain the
default. The performance is definitely better for moving a whole folder of
10MB high quality photo originals around, and most people have rsync.

Make sure the implementation isn't reading entire files into memory.

On Wed, Nov 14, 2012 at 11:41 AM, alexandre melard <[email protected]

wrote:

OK, I will add that :)
What do you think of the use of Finder and Filesystem classes by the way?

On 14 November 2012 17:39, Tom Boutell [email protected] wrote:

Thanks for the pull request! However as written this change will break
the
syncing of content back to the permanent folder on a later edit of an
existing object. Files that are removed from the collection in the
editor
need to be removed from the permanent folder. The syncFiles() method
must
do what it promises to do - if we ditch rsync then we must still provide
the functionality of rsync.

This is an example of code in my project that would break with this
change:

$fileUploader->syncFiles(
array('from_folder' => '/tmp/photos/' . $editId,
'to_folder' => '/photos/' . $posting->getId(),
'remove_from_folder' => true,
'create_to_folder' => true));


Reply to this email directly or view it on GitHub<
https://github.com/punkave/symfony2-file-uploader-bundle/pull/14#issuecomment-10373108>.


Reply to this email directly or view it on GitHubhttps://github.com//pull/14#issuecomment-10373186.

Tom Boutell
P'unk Avenue
215 755 1330
punkave.com
window.punkave.com

Alexandre Melard added 3 commits November 14, 2012 19:08
mirror : I added the possiblity to delete files in the $to directory
that are not in the $from directory
new mod will be in FileManagerSF2
Disable files deletion from target by setting the 'delete' option to
FALSE
Activate the SF2 class in services
No shell tools required used only SF2 components
@alexandre-melard
Copy link
Author

Ok, I have tested the whole, seems OK, I am not yet familiar with the unitary testing in SF2 so I did it old fashioned way...
Let me know what do you think.

By the way, when using the multipart posting on OSX php make apache SIGSEV I may open an issue to discuss this if you want... I had to switch to my windows box to test your bundle. I may have a look on the FileUpload services later on

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants