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.
Comment | File | Size | Author |
---|---|---|---|
#23 | media-view-modes-upgrade-2856433-23-full-do-not-test.patch | 2.45 KB | dsnopek |
#23 | media-view-modes-upgrade-2856433-23.patch | 1.39 KB | dsnopek |
|
Comments
Comment #2
natew CreditAttribution: natew commentedAttached 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.
Comment #3
sylus CreditAttribution: sylus commentedI think this makes sense but definitely needs some more reviews.
Comment #4
joseph.olstadWould it make more sense to make a new hook update instead ? In case someone ran it already and didn't have a backup?
Comment #5
joseph.olstadAh ok nevermind my last comment.
The improvement makes sense.
Comment #6
joseph.olstadFYI 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.
Comment #8
joseph.olstadFixed. 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.
Comment #9
sylus CreditAttribution: sylus commentedOh 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?
@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! :)
Comment #10
joseph.olstadAh Ya, I am running on 4 hrs sleep, probably should have waited a bit and reviewed after more sleep
Comment #11
joseph.olstadReview after 8hr brain wash
Comment #13
joseph.olstad@sylus you are correct about suspecting
$status
, appeared to be a typeo, natew meant to put$enabled
Comment #14
joseph.olstadtagged 7.x-2.0-rc9
Comment #16
joseph.olstadFound another issue, see commit for details.
yet another tag.
Comment #18
joseph.olstadOk, now its good, finally. Thanks
Comment #19
natew CreditAttribution: natew commentedI apologize about the typo/mistake, thanks for cleaning up the patch!
Comment #20
joseph.olstad@natew , I think there still is a problem with the changes.
I'll be re-checking this asap.
Comment #21
joseph.olstadLooks like I put in another type-o , an extra underscore .. Going to double check this all again.
Comment #22
dsnopekWe'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
Making a diff with all the committed changes looks like this:
There definitely appears to be an extra underscore added here (two appros lines in diff moved next to each other):
Comment #23
dsnopekHere'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!).
Comment #24
sylus CreditAttribution: sylus commentedThanks so much for taking a look at this @dsnopek! Will be taking a look at this today as well.
Comment #25
natew CreditAttribution: natew commentedThanks @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.
Comment #27
joseph.olstadThanks 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!
Comment #28
joseph.olstadhttps://www.drupal.org/project/media/releases/7.x-2.0-rc12