Merging all of the submodules into oEmbed (http://drupalcode.org/project/oembed.git/commit/847399f) greatly improved code readability and simplified the process of installing and configuring oEmbed. However, I believe that it would advantageous to split out only the Media integration for a number of reasons:

  • Further code readability improvements - the code for fields, filters, etc all pertains to core; none of the Media, Media Internet and File Entity integrations are necessary in order to use the base oEmbed module
  • Improved performance - while much of the integrations are in separate files which are only loaded when necessary, there are still a few lines of code which are loaded unnecessarily or check to see if the integration module exists
  • Dependencies - it isn't clear what modules oEmbed integrates with and there are issues with dynamically altering oEmbed's dependencies to require Media/File Entity 2.x (if one of the modules does not have a version, such as when they are checked out via git, they will fail requirements)
  • Extensibility - if a user wants to write their own Media + oEmbed integration or wants to use the oEmbed filter without adding oEmbed as a supported internet media provider then they can simply leave the Media: oEmbed submodule disabled vs. having to write custom code to disable its Media integrations.
CommentFileSizeAuthor
#1 split-out-media-integration-2226639-1.patch45.28 KBdevin carlson

Comments

devin carlson’s picture

devin carlson commented
Status: Active » Needs review
StatusFileSize
new split-out-media-integration-2226639-1.patch45.28 KB

A patch to only split out the Media integration into Media: oEmbed.

devin carlson’s picture

devin carlson commented
Status: Needs review » Fixed

Tested #1 with an existing install and verified that everything continued to work properly with the integration code moved to Media: oEmbed.

Committed #1 to oEmbed 7.x-1.x.

Anonymous’s picture

Anonymous (not verified) commented

This is a good idea.

I think you may have made the input filter optionally dependent on media module when previously it was dependent only on file entity with this patch. If you check out oembed.filter.inc, you'll see that `oembed_resolve_link()` and `oembed_filter_oembed_settings()` both check for file entity and inline modules.

If the file entity hooks are all moved to media_oembed, the filter needs to be dependent on it for this functionality that doesn't require media module at all.

How do you think this should be handled?

Anonymous’s picture

Anonymous (not verified) commented
Status: Fixed » Needs work
Anonymous’s picture

Anonymous (not verified) commented
Assigned: Unassigned » devin carlson

I have removed the obscure dependency of the oEmbed filter on file entity and inline. Please review #2239345. If that patch is all good, then *this* issue's patch is good too!

Anonymous’s picture

Anonymous (not verified) commented

(By the way, I don't see any evidence that this patch was committed despite what you said in #2.)

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

Hi, I was too looking for a way to turn off media integration and just use the input filter, just applied split-out-media-integration-2226639-1.patch to 7.x-1.x from git repository today and ran updb:

Oembed  7002  Rebuild the code registry in order to accommodate moved 
               submodules.

Tested and deployed on an internal site, does exactly what we need so far. Thanks for the patch!

If there aren't more work that need to be done, let's RTBC it and get it commited, or start a new issue for any related work.

sheldon rampton’s picture

sheldon rampton commented
Status: Needs review » Needs work

I've encountered a problem due to the fact that there is a separate media_oembed module project on Drupal.org, which I see was created by @Devin Carlson. I haven't reviewed the code of that module closely, but can see that it is closely related to the submodule that this patch defines for the oembed module.

The problem I encountered happened when I tried installing the latest release of the Drupal Commons distro, which includes both the oembed module and the media_oembed module in its codebase. This means that Drupal Commons contains two separate modules with the identical name media_oembed, which was causing my website to throw errors. Note: The Drupal Commons distro uses commit 9aa5303e889f170ddfac7ca6c9ba26299820ad1c of the oembed module, in which the media_oembed submodule still lives within a "deprecated" subdirectory. I therefore deleted the "deprecated" version module from oembed, and my site appears to be running normally now. However, it is still not clear to me which version of media_oembed I ought to be using -- the separate version or the version that Devin has added here. Development of that module ought to be happening in one place rather than in two separate projects to avoid this kind of confusion.

Anonymous’s picture

Anonymous (not verified) commented

+1 I too running a Drupal Commons site that I also had this patch applied on, the way I have done it is to delete the standalone media_oembed module, and use the media_oembed module that comes with the oembed module… so I have in the profiles/common/moduels/contrib directory:

oembed/
oembed/modules/media_oembed
media_oembed/ *REMOVED*

That’s with the patch applied, if that makes sense?

Without the patch, you have oembed/deprecated/media_oembed and media_oembed in the common/moduels/contrib directory, with deprecated/media_oembed set to hidden.

But yes I think we should stick with one media_oembed module. I am for using the bundled media_oembed module under oembed project as that allows me to have the oembed filter only without media integration, and for sites that I need media integration, I can just turn on the bundled media_oembed module.

Anonymous’s picture

Anonymous (not verified) commented
Priority: Normal » Major

Devin,

I did not realize there was another Media: oEmbed module. It would be really great if you could sort this out soon. This work is ready to commit (but please check #2239345 first) but I am not sure what to do about the fact that there's another module that is named the same as a module you want to re-create in this project.

mohrizmus’s picture

mohrizmus commented

+1 I too running a Drupal Commons site that I also had this patch applied on, the way I have done it is to delete the standalone media_oembed module, and use the media_oembed module that comes with the oembed module… so I have in the profiles/common/moduels/contrib directory:

Tq, your idea work for me too.

crag2012’s picture

crag2012 commented
Issue tags: +d7docs

I too have this problem and in reading this thread, it is not clear to what module this patch is applied.
Clarity for beginners and others new to operating and constructing Drupal sites is needed.
In this case can we say this patch is applied to module oEmbed or module Media_oEmbed.
I am using module Patch to carry this out as it seems the safest way to carry this out and to revert if required.
Finally Thank you Folks for putting time and effort into this CMS it is appreciated

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented

Can we change the name of the sub-module? It's conflicting with the existing media_oembed module. I think the sub-module should have a prefix indicating it's part of this oembed module.

socialnicheguru’s picture

socialnicheguru commented

I am running into this issue and could not figure out why I needed oembedcore when using media_oembed module (http://drupal.org/project/media_oembed).

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

did anyone by chance make a patch for this? if so, please provide.

joseph.olstad’s picture

joseph.olstad
Primary language French
commented

Ya this is confusing for people.

if you have this problem, follow the workarounds mentioned above.

astonvictor’s picture

astonvictor commented
Status: Needs work » Closed (outdated)

D7 reached its EOL back in January 2025, and there is no active release for D7 for this module anymore.
Development or support is not planned for D7. All D7-related issues are marked as outdated in a bunch.

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.