I ran into an issue upgrading a site with media (2.0beta7 -> 2.0-rc5) using the wetkit distribution (4.14 -> 4.15). After much investigation it appears that update 7205 doesn't check the status before migrating the configuration. This causes view modes to be set which were not previously set.

Specifically this commit: http://cgit.drupalcode.org/media/commit/?id=0e035bfd6aba4190be79df55d9f4...
and this issue: https://www.drupal.org/node/2841742

In my case it was causing images added in the wysiwyg to inadvertently have a pre-defined image style applied (100 x 100). Which didn't match the view mode applied when adding via media. It added configurations that were not present prior to upgrade.

Since I didn't have any "WYSIWYG view mode" on my file types I was able to truncate media_view_mode_wysiwyg as a solution. I would also suggest we may want to check to see if the "Media WYSIWYG View Modes" are checked/enabled before we migrate the configs. Patch to follow.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

natew created an issue. See original summary.

natew’s picture

Attached is a patch to add logic to see if the view modes are enabled before migrating the configurations. It also contains some coding standards fixes. This worked for my specific configurations and test case.

sylus’s picture

Status: Active » Needs review

I think this makes sense but definitely needs some more reviews.

joseph.olstad’s picture

Would it make more sense to make a new hook update instead ? In case someone ran it already and didn't have a backup?

joseph.olstad’s picture

Ah ok nevermind my last comment.
The improvement makes sense.

joseph.olstad’s picture

FYI natew, yes lots of changes in media however we are now very close to a 7.x-2.0 stable release . 6 months ago this queue was full of patches and patches requiring patches.

I will review your patch asap because at least one other person reported an upgrade issue that was probably related to this issue. IMHO it warrants yet another tag as the less bugs in the latest release the better. I've pushed hard to get this far. Because of the effort to get media to 7.x-2.0 stable, there are new releases of multiform, media_ckeditor, wysiwyg, and possibly others that I forgot. I actually became maintainer of several of these to get the whole media ecosystem working on tagged released and dependencies.

And just a bit of history, the original maintainer of media passed away, and his illness and untimely death was a big reason why the 7.x-2.0 release has taken this long. I am very pleased with the progress in recent months and now the issue queue has slowed down drastically as the module matures and has stabilized. This may be one of the last commits made before 7.x-2.0 is tagged. Knock on wood.

Thanks for your patch.

  • joseph.olstad committed 561ee1c on 7.x-2.x authored by natew
    Issue #2856433 by natew: Update 7205 doesn't check if file view modes...
joseph.olstad’s picture

Status: Needs review » Fixed

Fixed. No need to review this further because I already know that @sylus has ran this through his CI / Travis and Behat tests as well as the realworld tests already been done.

I'd say that this was sort of an edge case type issue as there hasn't been a flood of reports relating to this and there's already thousands of installs using rc3+

With that said, IMHO, better to fix, tag and release than to let a bug stick around on a tagged release.

sylus’s picture

Oh no I approved the the direction with checking the status before assignment and did a quick glance but set to needs review for someone to look over the patch itself.

Upon a thorough code review of the patch I can see how this will pass all of our tests (D.O and my own) because to my mind it prevents the code from even running?

+++ b/modules/media_wysiwyg/media_wysiwyg.install
@@ -170,30 +171,36 @@ function media_wysiwyg_update_7205() {
+    $enabled = variable_get('media_wysiwyg_view_mode_audio_wysiwyg_restricted_view_modes_status', FALSE);
+    if ($status) {
+      $wysiwyg_restricted_view_modes = variable_get("media_wysiwyg_view_mode_{$type->type}_wysiwyg_restricted_view_modes", array());
+      foreach ($wysiwyg_restricted_view_modes as $wysiwyg_restricted_view_mode) {
+        db_insert('media_restrict_wysiwyg')

@natew

Is this right? We are assigning to $enabled but then immediately checking with $status? Does $status exist earlier on in the code?

I won't be so bold to switch this issue as it is late and I might be missing something but just want to make sure.

Also thank you for your awesome work @joseph I can only imagine the amount of work it is to get media to stable but is definitely shaping up! :)

joseph.olstad’s picture

Ah Ya, I am running on 4 hrs sleep, probably should have waited a bit and reviewed after more sleep

joseph.olstad’s picture

Status: Fixed » Needs review

Review after 8hr brain wash

  • joseph.olstad committed 0d1c858 on 7.x-2.x authored by sylus
    Issue #2856433 by natew, joseph.olstad, sylus: Update 7205 doesn't check...
joseph.olstad’s picture

@sylus you are correct about suspecting $status, appeared to be a typeo, natew meant to put $enabled

joseph.olstad’s picture

Status: Needs review » Fixed

tagged 7.x-2.0-rc9

  • joseph.olstad committed a6395f1 on 7.x-2.x authored by sylus
    Issue #2856433 by natew, joseph.olstad, sylus: Update 7205 doesn't check...
joseph.olstad’s picture

Found another issue, see commit for details.
yet another tag.

  • joseph.olstad committed 7b0166c on 7.x-2.x authored by sylus
    Issue #2856433 by natew, joseph.olstad, sylus: Update 7205 doesn't check...
joseph.olstad’s picture

Ok, now its good, finally. Thanks

diff --git a/modules/media_wysiwyg/media_wysiwyg.install b/modules/media_wysiwyg/media_wysiwyg.install
index 715de78..304d57f 100644
--- a/modules/media_wysiwyg/media_wysiwyg.install
+++ b/modules/media_wysiwyg/media_wysiwyg.install
@@ -178,9 +178,9 @@ function media_wysiwyg_update_7205() {
   // Migrate the configuration from the old variables into the new DB tables.
   $types = file_type_load_all(TRUE);
   foreach ($types as $type) {
-    $enabled = variable_get('media_wysiwyg_view_mode_audio_wysiwyg_restricted_view_modes_status', FALSE);
-    if ($status) {
-      $wysiwyg_restricted_view_modes = variable_get("media_wysiwyg_view_mode_{$type->type}_wysiwyg_restricted_view_modes", array());
+    $enabled = variable_get("media_wysiwyg_view_mode_" . $type->type . "_file__wysiwyg_restricted_view_modes_status", FALSE);
+    if ($enabled) {
+      $wysiwyg_restricted_view_modes = variable_get("media_wysiwyg_view_mode_" . $type->type . "_file_wysiwyg_restricted_view_modes", array());
       foreach ($wysiwyg_restricted_view_modes as $wysiwyg_restricted_view_mode) {
         db_insert('media_restrict_wysiwyg')
           ->fields(array(
@@ -191,9 +191,9 @@ function media_wysiwyg_update_7205() {
       }
     }
 
-    $enabled = variable_get('media_wysiwyg_view_mode_audio_file_wysiwyg_view_mode_status');
+    $enabled = variable_get("media_wysiwyg_view_mode_" . $type->type . "_wysiwyg_view_mode_status");
     if ($enabled) {
-      $file_wysiwyg_view_mode = variable_get("media_wysiwyg_view_mode_{$type->type}_file_wysiwyg_view_mode", 'wysiwyg');
+      $file_wysiwyg_view_mode = variable_get("media_wysiwyg_view_mode_" . $type->type . "_file_wysiwyg_view_mode", 'wysiwyg');
       db_insert('media_view_mode_wysiwyg')
         ->fields(array(
           'type' => $type->type,
natew’s picture

I apologize about the typo/mistake, thanks for cleaning up the patch!

joseph.olstad’s picture

@natew , I think there still is a problem with the changes.

I'll be re-checking this asap.

joseph.olstad’s picture

Looks like I put in another type-o , an extra underscore .. Going to double check this all again.

dsnopek’s picture

We've had this reported in Panopoly, and we're trying to figure out how to address here:

#2856834: Incorrect image style used in WYSIWYG on updated sites

Looks like I put in another type-o , an extra underscore .. Going to double check this all again.

Making a diff with all the committed changes looks like this:

@@ -166,30 +171,36 @@ function media_wysiwyg_update_7205() {
 
   // Create the new configuration tables.
   if (!db_table_exists('media_restrict_wysiwyg')) {
-    db_create_table('media_restrict_wysiwyg',  $schema['media_restrict_wysiwyg']);
+    db_create_table('media_restrict_wysiwyg', $schema['media_restrict_wysiwyg']);
     db_create_table('media_view_mode_wysiwyg', $schema['media_view_mode_wysiwyg']);
   }
 
   // Migrate the configuration from the old variables into the new DB tables.
   $types = file_type_load_all(TRUE);
   foreach ($types as $type) {
-    $wysiwyg_restricted_view_modes = variable_get("media_wysiwyg_view_mode_{$type->type}_wysiwyg_restricted_view_modes", array());
-    foreach ($wysiwyg_restricted_view_modes as $wysiwyg_restricted_view_mode) {
-      db_insert('media_restrict_wysiwyg')
+    $enabled = variable_get("media_wysiwyg_view_mode_" . $type->type . "_file__wysiwyg_restricted_view_modes_status", FALSE);
+    if ($enabled) {
+      $wysiwyg_restricted_view_modes = variable_get("media_wysiwyg_view_mode_" . $type->type . "_file_wysiwyg_restricted_view_modes", array());
+      foreach ($wysiwyg_restricted_view_modes as $wysiwyg_restricted_view_mode) {
+        db_insert('media_restrict_wysiwyg')
+          ->fields(array(
+            'type' => $type->type,
+            'display' => $wysiwyg_restricted_view_mode,
+          ))
+          ->execute();
+      }
+    }
+
+    $enabled = variable_get("media_wysiwyg_view_mode_" . $type->type . "_wysiwyg_view_mode_status");
+    if ($enabled) {
+      $file_wysiwyg_view_mode = variable_get("media_wysiwyg_view_mode_" . $type->type . "_file_wysiwyg_view_mode", 'wysiwyg');
+      db_insert('media_view_mode_wysiwyg')
         ->fields(array(
           'type' => $type->type,
-          'display' => $wysiwyg_restricted_view_mode,
+          'view_mode' => $file_wysiwyg_view_mode,
         ))
         ->execute();
     }
-
-    $file_wysiwyg_view_mode = variable_get("media_wysiwyg_view_mode_{$type->type}_file_wysiwyg_view_mode", 'wysiwyg');
-    db_insert('media_view_mode_wysiwyg')
-      ->fields(array(
-        'type' => $type->type,
-        'view_mode' => $file_wysiwyg_view_mode,
-      ))
-      ->execute();
   }
 
   // Remove old configuration variables.

There definitely appears to be an extra underscore added here (two appros lines in diff moved next to each other):

-    $file_wysiwyg_view_mode = variable_get("media_wysiwyg_view_mode_{$type->type}_file_wysiwyg_view_mode", 'wysiwyg');
+    $enabled = variable_get("media_wysiwyg_view_mode_" . $type->type . "_file__wysiwyg_restricted_view_modes_status", FALSE);
dsnopek’s picture

Here's a new patch of latest 7.x-2.x to fix the extra underscore thing, but I also noticed that one of the variable_get() calls doesn't pass in a default value. Of course, the default default is NULL which also evaluates to falsey, for consistency and explicitness, it'd be good to pass in a value.

I've also attached a "full" patch, in case someone is looking to put this in a distribution .make file (like Panopoly!).

sylus’s picture

Thanks so much for taking a look at this @dsnopek! Will be taking a look at this today as well.

natew’s picture

Thanks @dsnopek I tried my original steps to recreate with the wetkit distro, with your patch and the latest 7.x-2.x media module. I didn't have any of the issues previously.

  • joseph.olstad committed 7d7aee2 on 7.x-2.x authored by dsnopek
    Issue #2856433 by dsnopek, natew: Update 7205 doesn't check if file view...
joseph.olstad’s picture

Status: Needs review » Fixed

Thanks everyone, I did some investigating as these variables haven't existed since media 7.x-2.0-beta7 , they were removed in 7.x-2.0-beta8 , so these issues only affect those that were upgrading from <= 7.x-2.0-beta7 .

The code related to this issue is FINALLY correct.

This warrants yet another tag. I swear we're ALMOST at 7.x-2.0 release. getting closer!

joseph.olstad’s picture

Status: Fixed » Closed (fixed)

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