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.
Comments
Comment #1
gambryIt'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.
Comment #2
gambryBelow 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.
Comment #3
das-peter CreditAttribution: das-peter at Cando commented@gambry That's a pretty nice solution to this issue! I've some nitpicks and further thoughts - feedback appreciated :)
Comments should end in a full stop.
I suggest we change the comment a bit:
What do you think?
I think we should use a
!empty($entity->bypass_mbp)
instead ofisset($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:
Comment #4
gambryWonderful 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.
Makes sense. Cleaner and more useful. And I agree with
mbp_bypass
name, so I would update it in the comments too.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.
Comment #5
das-peter CreditAttribution: das-peter at Cando commentedSounds great :) Looking forward to give the updated patch a test run. Thanks!
Comment #6
gambryAttached the patch, which includes:
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.
Comment #8
das-peter CreditAttribution: das-peter at Cando commented@gambry Fantastic :) I just reviewed the changes. Looks good to me. Found some coding standard nitpicks - see the coder (CodeSniffer) output below.
CodeSniffer Output:
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!
Comment #9
gambryI've always thought changing the issue status should be the maintainer.
Good to know. Thank you for your help and contribution!
Comment #10
das-peter CreditAttribution: das-peter at Cando commentedOn 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 :)