After uploading files I couldn't see them because there was public:// twice in the URI. I also got the error message that my file public://screen_shot.png could not be uploaded.

After debugging I came to the point where I just removed the scheme from the two occurrences in code.

Can you tell me why we really need the schemes in that places? It looks like media_browser_plus_construct_dir_path is already adding that to.

CommentFileSizeAuthor
#1 media_browser_plus-public-public-2492917-1.patch2.28 KByannickoo

Comments

yannickoo’s picture

yannickoo
Location Berlin
commented
Status: Active » Needs review
StatusFileSize
new media_browser_plus-public-public-2492917-1.patch2.28 KB
das-peter’s picture

das-peter commented
Status: Needs review » Needs work

media_browser_plus_construct_dir_path() should not add any stream wrappers, and as far as I can see it doesn't.
We need to rely on file_uri_scheme($file->uri) to ensure we rely on the stream wrapper defined by the entity. The entity could be public:// or private:// - and MBP handles folders for both locations and is not allowed messing with the location (that could be a major security issue otherwise).
While making sure I don't tell you non-sense I became aware, that media_browser_plus_move_physical_folder() still did an assumption on the used stream wrapper. This is fixed as of now: http://cgit.drupalcode.org/media_browser_plus/commit/?id=86aa82a

As of now there shouldn't be any location in the MBP code where the stream wrapper is hard coded. It always has to rely either on the entity or on media_get_local_stream_wrappers() when handling folders and such.
However, we still need to figure out where a duplication of the stream wrapper could happen, any chance you can debug further? Or provide detailed steps to replicate?

yannickoo’s picture

yannickoo
Location Berlin
commented

I think the problem was that the root folder was set to public://media – I'm guessing that this value should me a directory only like media, is that true?

das-peter’s picture

das-peter commented
Status: Needs work » Closed (works as designed)

Sounds about right - no stream wrappers there.

Status: Closed (works as designed) » Needs work

The last submitted patch, 1: media_browser_plus-public-public-2492917-1.patch, failed testing.

das-peter’s picture

das-peter commented
Status: Needs work » Closed (works as designed)

Gnaa, testbot craziness.