Problem/Motivation

The media token upgrade mechanism scans all entries in the {file_usage} table, but it's really only necessary to upgrade entries that belong to the 'media' module. This will in many cases drastically reduce the number of parsed entities during token upgrade.

I completely missed this little feat of the file_entity/media symbiosis.

Proposed resolution

Add a db condition for module == 'media' in media_wysiwyg_upgrade_content_tokens_batch().

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

kaare created an issue. See original summary.

kaare’s picture

Issue tags: +Performance, +Novice, +Quick fix
kaare’s picture

Assigned: Unassigned » kaare
Status: Needs work » Needs review
FileSize
707 bytes

This one-liner will drastically reduce the affected entities in large installations.

joseph.olstad’s picture

are documents and music files like .mp3 also type media? those can be embedded as well.
What about custom file types? I think those can be customized to be embedded, would have to double check.

kaare’s picture

Good questions and reasonable concerns.

I'm quite sure this is valid for other file types as well. as long as the token type == media and it has a fid it's counted as a managed file by media module and 'media' is the module owner, as set by _media_wysiwyg_filter_add_file_usage_from_fields(). Within it there are calls to file_usage_delete() and file_usage_add(), both with 'media' as module owner.

But to be sure, I extended the media_dev distro with a custom file type and inserted files of this type both on nodes as stand alone file fields, and in other nodes as wysiwyg tokens. I also added mp3s both in standalone file fields and within wysiwyg areas: In the database the file usage table tracks these files in wysiwyg text areas as owners of 'media'.

I'm quite sure this patch is correct.

wells’s picture

I have tested this patch against my database, stemming from Issue 2950150. This patch resolves the issue for my environment and cuts the number of batched entities from 250k to 311. I spot checked many of the updated nodes and everything appears to be as expected.

kaare’s picture

Thank you @wells for your feedback and testing. This revelation came while investigating your issue and I'm glad you tested it in your large installation. I imagined this simple patch will omit the troublesome entities during upgrade in your case. The drop in parsed entities was enormous for your case, as suspected. I'm ready to call this rtbc.

joseph.olstad’s picture

nice test results.
Just curious, did you have any other file entity content types created other than the default 'media' type (and configured for embedding)?
I imagine that those custom ones might not get upgraded in that case?

joseph.olstad’s picture

Assigned: kaare » joseph.olstad

looking into this now

kaare’s picture

Yes I did. I created a file entity bundle (type) called 'patch' with text/x-diff as mime and .patch/.diff as additional extensions in file settings. It was added as an allowed type in WYSIWYG under admin/config/media/browser. In addition I created a new content type name 'issue' with a new file field named 'field_patch' and media browser as widget. I then tested creating a new node (of type 'issue') and uploaded a patch here. I created a new node (of type 'article') and in the 'body' using wysiwyg media browser I uploaded a new patch and inserted it as media token. All this done in the 7.x-2.x media branch. I then switched to the media 7.x-3.x branch with this patch applied and ran the upgrade. Everything worked as planned. The 'issue' nodes wasn't touched, the articles with the 'patch' file bundle as token was upgraded along all other tokens. I inspected the file_usage table and asserted that the different 'patch' files belonged to different modules, and that the one inserted in wysiwyg had 'media' as module owner.

All good.

joseph.olstad’s picture

Assigned: joseph.olstad » kaare
Status: Needs review » Reviewed & tested by the community

ya just tested, I created a new file type called xyz , configured it for images, enabled wysiwyg embedding, embedded it, the file_usage shows 'media' despite the file type/bundle being xyz.
so yes, this is a good patch thanks double confirmed!

I was curious too lol

joseph.olstad’s picture

so we should also put this in the 4.x branch?

kaare’s picture

Yep. Forgot to add patch for that branch as well (code was moved to .update.inc

Status: Reviewed & tested by the community » Needs work
joseph.olstad’s picture

Status: Needs work » Reviewed & tested by the community

ok, so RTBC patch #13 for 4.x
and RTBC patch #3 for 3.x

Munga takk!

joseph.olstad’s picture

So ya Kaare, I'm trying to stay out of your way in the 3x and 4x branches, great work!

  • kaare committed c3c7770 on 7.x-3.x
    Issue #2962845 by kaare, joseph.olstad, wells: Media token upgrade only...

  • kaare committed 9c3ef82 on 7.x-4.x
    Issue #2962845 by kaare, joseph.olstad, wells: Media token upgrade only...
kaare’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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