File entity module compatibility

Shameless subscribe.

It was nice meeting you in Stockholm!

Yes - I need to look into this!

Yeah, totally needing integration >.<

#1732688: Integrate with Storage API media uses File entity in 2.x now

As media will become "just a frontend", it's file entities that we really want to support.

more shameless subscribe. any word on this lately?

I'd like word on this too. Seems upload works fine when using a File-field and Media selector, but the files aren't added to Media's library - however they can be imported using the Media Importer?

Would be interested to know if this can be implemented as well. The problem is that it's easy to use the Media File Selector widget and this makes things like image previews in the Edit form look broken.

But all files are uploaded fine via the media-widget, and storage-api performs sync/audit/cleanup correctly to all the containers. So the only thing that seems missing is the fact of the UI looking broken and the CMS not being as helpful as it could be.

If someone could detail why there's a problem (and I guess I can dig into the source to figure it out as well otherwise) I'm happy to help implement a solution.

Files uploaded through Storage API don't show in Media's library or at /admin/content/file because the storage streams are of type STREAM_WRAPPERS_HIDDEN. The following functions in file_entity.module and media.module (2.x) filter out hidden streams:

function file_entity_get_hidden_stream_wrappers() {
  return array_diff_key(file_get_stream_wrappers(STREAM_WRAPPERS_ALL), file_get_stream_wrappers(STREAM_WRAPPERS_VISIBLE));
}

function media_get_hidden_stream_wrappers() {
  return array_diff_key(file_get_stream_wrappers(STREAM_WRAPPERS_ALL), file_get_stream_wrappers(STREAM_WRAPPERS_VISIBLE));
}

Is it bad to make Storage streams visible?

Yep, bad idea. Instead, I've added a hook_query_media_browser_alter to remove media browser's query conditions which filter out files from hidden streams.

I've also added a storage class option to the default scheme selection at /admin/config/media/file-system, which applies to media uploads. This stream wrapper is WRITE_VISIBLE so appears in any file wrapper selection options, eg. filefield, as "Storage class (default file scheme)", as well as a storage class selector at the bottom of the stream options.

Files uploaded to a storage class via media browser or core bridge fields now appear in the media library. Files in the default file scheme also appear at /admin/content/file, but files uploaded via field-specific (core bridge) storage classes don't appear there (yet), since the file_entity_admin_files form doesn't provide a query alter hook. I suppose it could be achieved via a form alter.

Here's a patch against Storage 7.x-1.5, tested with Media / File Entity 7.x-2.0-beta7.

I was also getting "maximum nesting levels reached" recursion when viewing text fields with the "Media tags to markup" filter enabled - due to a node_load() call in storage_core_bridge_storage_access (line 428 of storage_core_bridge.module). I guess the node_load called the text filter again, which called storage_core_bridge_storage_access, which called node_load etc.

I've changed this to a db_select for the necessary node fields, patch attached.

Media patch works good for me!
I've added the node->type to the recursion patch as node_access was giving a notice that it wasn't there.

Patch in #13 worked fine for me.

Patched against storage_api 7.x-1.5.

Tested with file_entity 7.x-2.0-alpha2+6-dev & media 7.x-2.0-alpha2+10-dev.

Ooops, didn't mean to leave RTBC set.

Been running this for a while without issue now.

We've been running this for awhile now as well with no issues. There were some code standards issues that should be addressed. Also we were using patches in #10 and #3 which should go together. I've combined those below as well as provided an interdiff.

Setting back to needs review since there is a new patch but this probably should still be RTBC once someone reviews to make sure this doesn't introduce any regressions.

Back to needs review.

@kbasarab There is no patch in #3. Do you mean #13?

Patch in #17 working well for us under very active use.

We've been using #10 and #13 on two production sites for a few months with no issues.

I've just installed #17 on a site in development and it applies/works perfectly.

Though the absolute paths didn't play nicely with our environment so I've re-rolled to be run relative to the module.

I have applied the patch and i'm uploading a video through video module and want to convert it with the integrated zencoder api.

If the video gets posted pack i get the error

PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'storage-file-default-scheme://videos/converted/231/facebook_1_1_' for key 'uri': INSERT INTO {storage_core_bridge} (storage_id, uri) VALUES (:db_insert_placeholder_0, :db_insert_placeholder_1); Array ( [:db_insert_placeholder_0] => 211 [:db_insert_placeholder_1] => storage-file-default-scheme://videos/converted/231/facebook_1_1_mp4_1409254057.mp4 ) in DrupalStorageStreamWrapper->stream_close() (line 572 of <httpdocs>/sites/all/modules/storage_api/core_bridge/storage_core_bridge.module).

There are many coding standards that this module doesn't abide by. It's going to be a long battle to get this thing up to snuff. The module name doesn't even match the project name which bugs me to no end. Patch in #17 has a lot of code changes.

Before I pick up these patches and start testing them. I need to know what the problem is. The patch in #17 has a lot of hooks that seem dependent on the Media module and I rather not go down that path if possible. Also I have never used the media module for anything so I am ignorant there.. Storage API as you know works with the core file processes so I would like to find a path between keeping Storage API non-reliant on any other module and how to possibly support the media module.

I also just came in as a co-maintainer in the last 60 days and there is an enormous mountain of work to be done on Storage API. I personally want to address coding standards before I start bringing in new features as that's a very big pet-peeve of mine.

@Perignon:
If thats about my report in #22 than please notice that it was about the video module not the media module.

@fox_01 Well note this thread is about the media module :-D

@Perinon: I know but what i was trying to say was if i use this patch with the video module it will break its conversion function. Even if the patch was not meant for video module its an incompatiblity that ships with the patch so i think we should keep this in mind.

@fox_01 Thanks for the note.

@Perignon, regarding #23, please let me know if there's anything I (and others at my company) can do to help get this up to par to be committed. We've been using media (and the file entity) with the storage API for some time on production sites. While there's inherent frustrations given the minimal support we feel that the combination is a perfect fit -- especially for high availability environments where setting up things like GlusterFS, etc. is overkill. Also, file entity is really the key to media and it's something that's being introduced into core in D8 so I'd imagine more will gravitate to it as awareness is generated.

Point-being, we see this as an important component of the Storage API if it's aimed at having a future in Drupal -- which we hope it is.

Related to #22, we'd opened an issue for a similar bug (#2260175: Integrity constraint violation: 1062 Duplicate entry 'storage_core_bridge:image_style:2-1'). Though it's not 100% clear whether this patch is causing the problem since there seems to be others with the same issue who aren't using the patch.

@dwatts3624 I took the time to read through this entire issue and all the patches. I do know about File Entity in D8 and that's a good thing. And I also want to support the Media module because it is a popular module.

Here are my thoughts. The code here creates a dependency and Storage API had no dependencies to function out of the box. Adding this code would force someone to install Media module to use the core Storage API and that is what I don't think we should do. Case in point is one of my own sites, I use Storage API but I have no use for Media Module at all. I would rather this built out as a submodule that someone can choose to use or not - even delete if they wanted too.

Thoughts?

Hey @Perignon. Thanks for the reply here.

Based on your comments I'd started creating a sub module called storage_file_entity_bridge. As I was learning more about the work everyone on this thread has contributed to by reviewing the patch line-by-line to ensure the sub-module wouldn't remove any functionality, I'd realized that most of the code doesn't actually apply directly to media & file_entity. It's simply giving us a method to route the entire filesystem (which the file entity used) to storage which is actually preferential (based on our typical build, at least). This is actually explained in #10. And I've attached a screen shot of the primary UI change (among others) that the patch makes.

I should also note that the screen shot was taken from a vanilla Drupal build. The only thing it contains is storage_api with the patch and everything works well. So it's really not at all dependent on file_entity and media. It just enables them to work with it.

If you'd like, I can post the sub-module I'd created but, logically, it makes sense to keep it part of the core bridge in my opinion.

The only thing media-specific from what I can see is the last hook (storage_core_bridge_query_media_browser_alter) which won't trigger if media isn't installed. And it's not uncommon to include in base modules to support others.

Lastly, @fox_01, I wonder if your issue is actually one with the video module. It's trying to save 'storage-file-default-scheme://videos/converted/231/facebook_1_1_' so something seems to be parsing out the 'mp4' string in the filename thinking it's the end of it. I tried looking through storage and it doesn't seem to manipulate filenames other than the stream (e.g. storage-file-default-scheme://). Though @Perignon could better speak to that.

Does the Zencoder callback work without storage_api when using filenames with the relevant extension in the name?

Sub module would be awesome. That maintains the agnostic stance of the main part of the module.

If you have code for a submodule contribute it as a patch.

My argument from the patches in this tread is they would require the media module. Which by my own use-case would require me to install a module I do not need or want.

Hey @dwatts3624. Any update on the sub module you started to work in? Clearly the manatainers won't go for the idea of including this functionality in the core module and it would be great to have Media module functionality supported.

Cheers.

#21 doesn't apply on HEAD. Reroll attached. Fixed some indentation and added comments.

ps. I know we want this in a submodule. Just wanted to test the functionality.

@jonhattan Thanks for the work!

@jonhattan I could help add this as a submodule, but I have a question first.

With this patch, when editing a File field with the Media browser widget, it doesn't seem possible to set the upload destination to a selected Storage class (Everything), only Storage class (default file scheme). Attempting to do so gives the error:

The upload destination must be one of the allowed schemes.

It's not clear to me yet where files uploaded to Storage class (default file scheme) will go?

it doesn't seem possible to set the upload destination to a selected Storage class (Everything),

Precisely this patch does a workaround on this by providing a new way of core integration: Storage class (default file scheme).

@jonhattan Yes that makes sense now. BTW from IRC, I'm still getting the issue where the image styles are not rendering properly until cron run (the image path is like <img src="http://local.mysite/system/storage/serve/191/480_0.jpeg?itok=KceGtBtO" alt=""> and does not seem to load from local storage even though it's the Initial container for the Everything class). However I haven't isolated it to this patch yet. Any other thoughts on this would be helpful though.

@Perignon The more I think about the idea of a submodule for file_entity integration, it does not seem like a good idea. Most of this patch adds file_default_scheme support, which is not specific to file_entity (so would not be appropriate to add in a file_entity submodule. All of that should go in the storage_core_bridge module instead). The only thing specific to file_entity is one hook implementation, which does not seem worth making a new module for. Many major contrib modules implement hooks defined by other contrib modules, so it seems best to leave this patch structured as-is. Does thinking about it this way make sense to you?

re #39. I agree with you @scottrigby. core_bridge submodule is really an integration with fields. field_bridge would be a better name for it.

OTOH, the patch here provides a new, visible, stream wrapper, that is used system wide. This should be the real core_bridge, or file_bridge or something like that. I know changing the name of current core_bridge to field_bridge, and providing a new core_bridge module sounds like a mess, but this is the thing.

In any case, I think that this patch should be a separate module (submodule or new project) not because it provides media/file_entity integration but because it provides an alternative core integration. I'm still doing some tests on a fresh site to understand in deep what this patch does, but if I'm not wrong, it is useful on its own without the need of core_bridge functionality.

@scottrigby My suggestion and support for a submodule is due to the dependency on Media module. Yes I know many modules that depend on other modules (I maintain a few here on D.O) but those dependencies are needed for the core operation of the module, they could not exist without them. Storage API is a functioning module and does it's core job without a dependency on Media module and I want to maintain that agnostic posture of Storage API. My personal use of Storage API is an example. I wanted and needed Storage API for my site, hence why I became a co-maintainer; but i have no need nor desire to install Media module, which is a very heavy module I might add. I want to give the user a choice to use Media module, not force it. This is why I support a submodule but not integrating it into the core portion of the module. I am even ok letting letting this patch stay in place and linking it from the product description page if necessary if that is the best way to provide that freedom to choose to the user.

@Perignon Ah I see what you mean. But implementing a hook doesn't create a dependency on the module defining the hook. If the module is enabled and invokes the hook, then your implementation will be called – if not, then not. So to be clear, with this patch storage, and storage_core_bridge can be enabled without requiring Media at all. But if Media does exist in the system, storage_core_bridge should still work.

One note, if you want to keep code separated, if you would like we could create inc files per contrib module (in this case storage/storage_core_bridge/file_entity.inc, etc) where we keep the relevant hooks, and just include_once from storage_core_bridge.module? Or if you do prefer to have contrib module code not part of a "core bridge" module at all (because of conceptual cleanliness), we could still make a submodule per contrib module in the project root (in this case storage/contrib/file_entity/file_entity.module, etc) even though it only contains one hook implementation.

Let me know which you prefer and I'll update the patch :)

Also @jonhattan I also agree on naming suggestions in #40 (renaming core_bridge to field_bridge or file_bridge), and it wouldn't be a mess as long as a new minor branch is created (7.x-2.x etc).

But do you really think the patch here can stand on it's own? I started making it into it's own submodule, until I realized (or it seemed) this new functionality in this patch depends on storage_core_bridge anyway. But am I wrong about that?

Following #40, I've created a separate module to try and untangle all of this. It provides a single stream wrapper that can be used system wide or per field instance. It is a simple alternative to core_bridge. https://www.drupal.org/project/storage_api_stream_wrapper

I will add a link to your module on the project page!

@scottrigby As long as no dependencies are created on other modules. I don't oppose the changes. I hate requirements creep more than anything else and I hate to impose a dependency on someone that they don't need unless it is required to function. And I really take a hard look if that requirement is real, valid, and completely necessary. Drupal module creep can get very out of hand and it's one of the black eyes that Drupal has as I see this in my own projects - my main site has 85 contrib modules to run as an eCommerce and blog website - not including features and custom modules! Adding those I am at 107 modules.

Bump. IMO this issue should be closed as won't fix in favour of https://www.drupal.org/project/storage_api_stream_wrapper

I've added thoroughly instructions to migrate from storage_core_bridge, with this patch applied or not. http://cgit.drupalcode.org/storage_api_stream_wrapper/tree/README.md

re #45 please do!

#45 is done.

Patch #34 is working.
Updated that patch to work with Storage_api 1.8.