Hi.

This module implaments media browsers.. And this have nice ui.
But module mek real file folder and move files into that. It's a dangerous work.

If user inserted link to media and chaged media folder, post's link will be broken.
and When create folder for non latin name (ex. cjk), It will make folder non latin folder name.
Non latin folder name will not work properly when use php file system functions. (Because php has bug for non-latin characters.)

I think module should provide to option for logical folder that not create real folder.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

MiroslavBanov’s picture

Agreed. If I could use only one media directory, and "media_folders" was just a vocabulary with the same filtering logic, this would solve many problems for me. The physical directory is just asking for trouble. But the maintainer of the module chose to make it like this so it probably won't be changed.

das-peter’s picture

If user inserted link to media and changed media folder, post's link will be broken.

This sounds more like abusing the media system. The system is designed to "keep track" on moving files - but that only works if it's used properly. E.g. you should use a file/image field and the related file displays, this combination uses the file id to deal with and not, a probably volatile, file path.
For WYSIWYG editors there's the media filter which does the same, it uses the file id and replace the token later by the current path when the content is displayed. (If you use the ckeditor module make sure you patch it #1504696: Integration with CKEditor module)

However, for some people it might still make sense to disable folders. I don't see why that couldn't become configurable - patches welcome.

ndobromirov’s picture

Implemented a patch that adds a checkbox in the configuration form that governs whether to use virtual folders or not.
The default value is: Virtual folders disabled.

When the functionality was implemented, with the new management of files, the root folder rename breaks.
The old method for moving the root folder will not work in sites with lot's of media associated files, because the files copy/move happens during the submit operation.

Implemented batch operation method that works in both cases (virtual and not), but it probably needs more testing.

das-peter’s picture

Please give the MBP version 3.x http://drupal.org/node/1963804 a try. I re-wrote the whole module, and added an option to enable / disable the folder management right away...

ndobromirov’s picture

Found a bug int the last patch, resulting in the creation of 'public:' folder under Drupal's root.
This folder makes public:// stream wrapper to stop working properly in PHP 5.3+.
Submitting a new patch against revision: d1d96d9b58d27d1db29a1d2cf4a9074e9328d813.

Grayside’s picture

@das-peter, I tried the 3.x version and the setting is incomplete.

* Various places around the codebase are still doing checks and possibly trying to create new directories.
* Files will potentially be "moved".
* Current behavior will move a file to the root of the configured MBP directory even if they already have a nested file location.

The attached patch (which is for 7.x-3.x only) simply validates the move operation against the new variable setting before anything else is done. If you don't want files managed by MBP, there's no reason to go any further that I could decipher.

MiroslavBanov’s picture

Version: 7.x-2.x-dev » 7.x-3.x-dev

Changing the issue version to 3.x. There is much more interest in developing the 3.x branch anyways.

Edit: In my opinion, there shouldn't even be an option to mange physical directories, as it is hardly useful and very problematic.

das-peter’s picture

Version: 7.x-3.x-dev » 7.x-2.x-dev

@Grayside Thanks for the patch.
Patch from #6 makes sense & looks good: committed

@MiroslavBanov: Someone who uses MBP is likely to have a more complex file structure and thus I think it's not a to strange assumption that it would help to manage the files in the physical storage too.

Setting version back to 7.x-2.x and keeping issue status "Active" for now.

ndobromirov’s picture

Are there any known issues in the 2.x version of the patch now?
Or we can mark it as RTBC?

kenorb’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, 6: 1939210-6-media_browser_plus-7.x-3.x.patch, failed testing.

salvis’s picture

Status: Needs work » Closed (outdated)