Here is the PSA:
https://drupal.org/node/2169767
Apparently, any version before 7.x-2.0-alpha3+37-dev is at risk and we package 7.x-2.0-alpha3+12-dev.
Every update to Media usually leads to issues and changes, so, it'd be great to make some Behat tests for all the Media functionality we depend on first. However, that might not be practical if no one has the bandwidth to figure out how to test WYSIWYG.
According to @erophobe we need to update media_youtube to the latest -dev also, because it causes a fatal error due to a call to media_access(): #1823376: media_access() is deceprated and will be removed
| Comment | File | Size | Author |
|---|---|---|---|
| #18 | panopoly_images-media-upgrade-2171121-18.patch | 972 bytes | dsnopek |
| #18 | panopoly_widgets-media-upgrade-2171121-18.patch | 7.17 KB | dsnopek |
| #18 | panopoly-media-tests-2171121-18.patch | 15.63 KB | dsnopek |
Comments
Comment #1
lsolesen commentedMaybe this is of use:
- http://singlebrook.com/blog/testing-imce-wysiwyg-integration-in-drupal-u...
- http://previousnext.com.au/blog/custom-step-definitions-drupal-drupalext...
- https://github.com/openscholar/openscholar/pull/1291
Comment #2
lsolesen commentedThere is a patch in the drupalextension making it possible to test for WYSIWYG:
- https://drupal.org/node/1826016#comment-form
Also OpenScholar has a test:
https://github.com/openscholar/openscholar/blob/SCHOLAR-3.x/openscholar/...
Comment #3
lsolesen commentedAnd @eliza did some work here, it seems: http://drupalcode.org/sandbox/eliza411/2089669.git/tree/7fdd1e504bd0aa8d...
Comment #4
dsnopekThat's some awesome stuff! Thanks for finding it. :-) I'll hopefully be able to look into this more some time later this week.
Comment #5
lsolesen commentedDrawing on work from @eliza - and the test is not passing yet - https://github.com/lsolesen/panopoly/tree/wysiwyg-media. Please help out.
Comment #6
dsnopek@lsolesen: Awesome, thanks! I'll try to take a look at this soon (probably next week) so we can move forward on the security fix.
Comment #7
dsnopekAccording to Mike Potter, this is the revision that 2.0-alpha3+37 refers to:
http://drupalcode.org/project/media.git/commit/1f46a9af74435a07ce054f661...
Comment #8
dsnopekComment #9
dsnopekComment #10
barrapontoWe're two months behind a security release. Can we schedule a sprint and get this done?
Comment #11
dsnopekI've added this to the hitlist for 1.3: #2219787: Panopoly 1.4 hitlist
Since the main blocker here is testing and that is a big interest of mine, I'll assign it to myself. My goal is to really dig into this on the weekend!
Comment #12
lsolesen commentedSounds good dsnopek :) Something happened with new permissions and new modules (something to do with the wysiwyg integration) between the releases, I am pretty sure. Commons recent update might give a clue: http://drupalcode.org/project/commons.git/commitdiff/7ef8460?hp=360826da.... I have seen that the maintainer of Commons also commits to Media so they might be of help.
Comment #13
cboyden commentedI've got a test that verifies two things: text formatting buttons such as Bold and Italic, and image insertion using the Media module. It only works if you run Chrome, not Firefox. (It should come with a trigger warning for hacky Javascript to get around browser quirks.)
I've updated the behat.template.yml with filters and a new "chrome" profile so anyone running the default profile will not see failures due to Firefox misbehavior.
To run the new test:
bin/behat -p chrome features/wysiwyg.featureThis test still needs a cleanup function to delete the image afterward.
Patch attached.
Comment #14
cboyden commentedComment #15
cboyden commentedThis test is just a proof of concept, getting the Selenium-run browsers to behave the same way they do for a live person using the interface. The test - and a bunch of others I've seen - suffers from exposing too much implementation detail in the scenarios. Nobody using Media/WYSIWYG would really think through "I switch to the iframe," it's not apparent that iframes are in use and that kind of step shouldn't be exposed in a behavior-driven scenario. An editor is just going to see a "Browse..." button and click it.
So next step, in addition to the cleanup function, would be to build the Media/WYSIWYG subcontexts so they can do the frame-switching behind the scenes. Then the step could be written
And I attach the file "panopoly.png" to the "Upload" field in the Media browseror some such, and it would make a lot more sense as a scenario.
Comment #16
dsnopek@cboyden: Thanks for all your hard work on this issue! Your code has definitely helped get over some serious problems. :-)
I've created a new version of the patch which works with Firefox. Appearently, it's selecting text that doesn't work in Firefox, so I switched to a pattern where we click a button and insert text, rather than select text and click a button. Also, I've switched to using the WYSIWYG modules API, which will hopefully help us do cross-editor testing for the transition to CKEditor.
FYI, this works against the version of Media currently in Panopoly. Updating to the revision we're targeting with this issue actually causes it to fail - which is a good thing! It's pointing out something that is broken and we need to fix. :-)
So, next steps:
I'm hoping to continue working on this today.
Comment #17
dsnopekAnd marking as "Needs work" because there's more I plan to do.
Comment #18
dsnopekMade some more progress! This ended up taking much longer than I had hoped because I got stuck on a legimate bug with YouTube and Firefox in the current version of Panopoly. This required a patch from the Media issue queue to fix.
Anyway, attached are three patches: one to the main Panopoly repo with the tests, and changes required to update Media for both panopoly_widgets and panopoly_images.
All that's left is to try and generalize the steps per comment #15. I'll do this tonight or tomorrow and then commit. :-)
Comment #20
dsnopekI spent a lot of time thinking about this, and in the end, I decided not to try and generalize the steps like described in #15... I would have had to create at least a half dozen duplicate steps that just had "in the Media Browser" or "in the Media Style Selector" added at the end and that just felt icky. So, I committed it as it was with the "switch to frame" steps. But we can always refactor it later!
In any case, the update is committed:
http://drupalcode.org/project/panopoly.git/commitdiff/18a2ccbdad8c8064aa...
Thanks so much to @cboyden and @lsolesen for the work you put into this issue!
Here is the Travis-CI build:
https://travis-ci.org/lsolesen/panopoly/builds/21350003
I'm really hoping it'll pass the first time through. I did a little light testing on Travis-CI, but only with the current version of Media and not with the upgrade testing stuff...
Comment #22
cboyden commentedExcellent, it's great that you found a way for the test to work without specifying headless or a specific browser.
In the future we might need to test selecting - not inserting - specific text inside a WYSIWYG, in which case we'll have to revisit. But that'll be a new issue when it happens.
For the refactoring question - I have no idea how this would work, but might it be possible for the subcontext to set some kind of session variable to keep track of when to switch frames?