Modules should be able to bypass media file processing. Some files should remain invisible to the media browser.

These and other files may not expect the file->uri value to be changed by hook_file_presave, which is called by file_save after all other processing in file_save_data. The media_browser_plus_file_presave code always changes the uri.

The link_favicon_formatter code looks for its icons, which should not be placed in the media browser directories, ONLY in the link_favicon_formatter_favicons directory. It builds the uri using that directory, and calls file_save_data to save or replace the file. The replace parameter is FILE_EXISTS_REPLACE. If the file exists in link_favicon_formatter_favicons when file_save_data is called, it is deleted from that directory. (I had hoped that putting a copy of the file there would bypass the problem code.) It always creates a new icon file, whenever a link_favicon is viewed, and places the new file in the media browser directory where it cannot find it. I'm going to do some surgery on the link_favicon_formatter code to bypass all saves, priming it with the files I need. Ick.

The real solution probably requires the media library, or at least the media prowser plus, to use a subdirectory of the public directory so media and other files cannot comingle and it would know which directories were subject to uri modification.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

gambry’s picture

It's probably related to this comment.

As I said there entity without a term but with a specific folder should NOT be changed from media_browser_plus.

gambry’s picture

Below the patch for both 7.x-3.x-dev and the current 7.x-3.0-beta3.

The patch check if the current $entity->uri is the root of the scheme. If it's not it returns without running the mbp file_presave actions.

Also it checks for an eventual $entity->bypass_mbp attribute, which can help future contrib module to bypass the actions immediately, even if $entity->uri is in the scheme root.

das-peter’s picture

Status: Active » Needs work

@gambry That's a pretty nice solution to this issue! I've some nitpicks and further thoughts - feedback appreciated :)

+++ b/media_browser_plus.file.inc
@@ -8,6 +8,12 @@
+  // Return if if bypass_mbp is set
+  // or there isn't a field_folder term BUT there is already a default folder

Comments should end in a full stop.
I suggest we change the comment a bit:

  // MBP folder processing is skipped if:
  // - Manually requested by adding the property bypass_mbp to the file entity.
  // - The file has no folder set and has a path in the filesystem defined.

What do you think?

+++ b/media_browser_plus.file.inc
@@ -8,6 +8,12 @@
+  if (isset($entity->bypass_mbp) || (empty($entity->field_folder[LANGUAGE_NONE][0]['tid']) && isset($entity->uri) && file_uri_scheme($entity->uri) . '://' !== $entity->uri))

I think we should use a !empty($entity->bypass_mbp) instead of isset($entity->bypass_mbp). That way $entity->bypass_mbp = FALSE; won't trigger inconsistent behaviour.
Another thing I thought of is changing the naming to mbp_bypass just for the sake of having a module prefix.

Last but not least we should add tests for the patch. The current tests pass with the patch applied.
We should test:

  • The manually set property is working.
  • The manually set folder approach is working.
  • Adjusting the media root won't change the file path: If the media root is changed all the files are processed. Here we might want to check what happens if a manually set folder is in the same path as an existing mbp folder!
gambry’s picture

Wonderful feedback!
Sorry for the mess (and the typos), part of the code is coming from a custom module, but then I preferred to search on the MBP issues and propose a patch.

Comments changes make sense.

Comments should end in a full stop.
I suggest we change the comment a bit:
[...]
What do you think?

Makes sense. Cleaner and more useful. And I agree with mbp_bypass name, so I would update it in the comments too.

I think we should use a !empty($entity->bypass_mbp) instead of isset($entity->bypass_mbp).

My original idea was to bypass the MBP hook if the property was just set, but I do understand the expression $entity->bypass_mbp = FALSE - logically - suggests to not bypass, so more than happy to use !empty().

I'll update the patch and working on the tests today as soon as possible, if everything written above is fine for you.

das-peter’s picture

Sounds great :) Looking forward to give the updated patch a test run. Thanks!

gambry’s picture

Attached the patch, which includes:

  • Comments copy and format reviewed
  • Code updated
  • Tests

I haven't included the last test Adjusting the media root won't change the file path, as I wasn't sure how to approach to that test, if updating the root_folder test or creating a new one, and - mainly - the check should be unrelated from the bypass feature.
e.g. I can imagine a situation where the directory already exists before installing MBP and a MBP folder with the same name is created later on.

Thoughts?

I'm also NOT including the updated patch for the 7.x-3.0-beta3, if needed I'll do.

  • das-peter committed 3b0bf93 on 7.x-3.x authored by gambry
    Issue #2252223 by gambry, das-peter: Modules should be able to bypass...
das-peter’s picture

Status: Needs work » Fixed

@gambry Fantastic :) I just reviewed the changes. Looks good to me. Found some coding standard nitpicks - see the coder (CodeSniffer) output below.
CodeSniffer Output:

FILE: ...ules\media_browser_plus\tests\media_browser_plus.mbp_bypass.test
----------------------------------------------------------------------
FOUND 7 ERRORS AFFECTING 4 LINES
----------------------------------------------------------------------
  51 | ERROR | [x] Functions must not contain multiple empty lines in
     |       |     a row; found 2 empty lines
  75 | ERROR | [x] Expected one space after the comma, 0 found
  75 | ERROR | [x] TRUE, FALSE and NULL must be uppercase; expected
     |       |     "NULL" but found "null"
  75 | ERROR | [x] Expected one space after the comma, 0 found
 120 | ERROR | [x] Doc comment short description must end with a full
     |       |     stop
 120 | ERROR | [ ] Doc comment short description must be on a single
     |       |     line, further text should be a separate paragraph
 141 | ERROR | [x] The closing brace for the class must have an empty
     |       |     line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 6 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

I fixed those nitpicks myself - no point in making another loop for that.
Tests all pass - so I'd say good to go.
Patch committed and pushed: http://cgit.drupalcode.org/media_browser_plus/commit/?id=3b0bf93

Last remark: Please use the issue status to indicate if there's a patch to review ("Needs review"). Otherwise your valuable patches might won't get the attention they deserve!

gambry’s picture

I've always thought changing the issue status should be the maintainer.

Good to know. Thank you for your help and contribution!

das-peter’s picture

I've always thought changing the issue status should be the maintainer.

On the contrary: I, as maintainer, rely heavily on the status set by the contributors. To be honest I quite often scan for "needs review" tickets first as it's often a sign that an issue has a certain level of feedback and consideration - besides having a patch. Even better is an RTBCed (reviewed & tested by the community) issue that's usually an indicator that the patch / solution was reviewed by a bunch of people and worked for them.
All that allows me to focus my limited time on issues that most likely can be fixed and all users of the module will benefit of.
Just to have a complete response to this here's the link to the d.o. guideline which explains that too: https://www.drupal.org/issue-queue/how-to#Status

So thanks again :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.