One goal of the patch is to more easily enable other modules to add their own plugins. However, CKEditor also provides its own plugins. We should remove any plugins that are module dependent but keep the ones that are internal.
Additionally it moves the plugin definitions to a new file ckeditor.ckeditor.inc and documents the new hook_ckeditor_plugins_alter in ckeditor.api.php.
Original report
A patch for Media was recently committed - it came with a companion patch for CKEditor that does many things (declares hooks in hook_hook_info, adds an alter function, moves code around, etc). It was originally written by Devin Carlson. See: #1504696: Integration with CKEditor module
Specifically, where a maintainer of this module asks for the patch to be filed against CKEditor. https://drupal.org/comment/8243115#comment-8243115
The patch in question is already uploaded to D.O. here: https://drupal.org/files/issues/ckeditor-accomodate-latest-media-changes...
I am reposting it to this issue so that it will go up against any automated tests that may be set up.
If this patch is committed, proper attribution should be to Devin Carlson: https://drupal.org/user/290182
Comment | File | Size | Author |
---|---|---|---|
#166 | interdiff-2159403-141-166.txt | 6.5 KB | aDarkling |
#166 | make_ckeditor_plugin-2159403-166.patch | 23.66 KB | aDarkling |
#89 | interdiff.txt | 2.83 KB | micbar |
Comments
Comment #1
deggertsen CreditAttribution: deggertsen commentedI think this should be RTBC since it has already been reviewed and tested. See #1504696: Integration with CKEditor module
Comment #2
jcisio CreditAttribution: jcisio commentedI was trying to review the patch, but it turned out that it would took more time than I'd thought. I don't think this patch was reviewed enough in CKEditor as it has a large impact on how it touches files. It needs test and review on admin UI and Features export to see if there are changes.
Comment #3
saltednutI have seen threee issues with this patch, 2 that are minor, 1 major:
1. When editing CKEditor appearance, ie: adding or removing buttons. After saving, the 'Media' button disappears from the actual rendered WYSIWYG. I can see it on the CKEditor profile settings, but its gone from the Editor itself.
The only way I've found to get around this is to go into Features and export the CKEditor profile. I then can update the export via text editing, and add in 'Media' manually to the exportable. Then reverting the Feature makes the 'Media' button return to the WYSIWYG. Unsure if that is something Media has to fix or if its on the CKEditor side of things.
2. There are now two checkboxes for inserting Drupal media in the plugin settings. One that is checked by default, "Plugin for inserting images from Drupal media module" and another that is not checked, and is seemingly unnecessary, "Plugin for inserting Drupal embeded media"
There is also a somewhat major issue with the ACF.
3. Basically, the Media embed codes get wiped upon editing an existing node with media inside it. Workflow to repeat:
This can be avoided by simply turning off the Advanced Content Filter.
Comment #4
das-peter CreditAttribution: das-peter commented@brantwynn I've tested the patch and after some patching it seemed to work as intended. Created patches:
#1995030: Support for WYSIWYG in summary
#2164823: Fix for replacePlaceholderWithToken in media_wysiwyg.filter.js
#2164831: Fix for CKEditor ACF (Advanced Content Filtering) configuration
I did a visual review of the patch too, but I found just some nitpicks. The rest looks quite good to me.
Could you check if the problems you've described are gone with the patched I've created? (1 & 2 weren't occurring in my installation, but 3, did)
Nitpicks:
A comma should follow the last multiline array item.
TRUE, FALSE and NULL must be uppercase; expected "FALSE" but
| | found "false"
Concatenating translatable strings is not allowed, use
placeholders instead and only one string literal
- No space before comment text; expected
- Inline comments must start with a capital letter.
- Inline comments must end in full-stops, exclamation marks, or question marks.
Comment #5
jcisio CreditAttribution: jcisio commentedI don't think we bother anything else except that whether the CKEditor plugin system, the admin UI and exported config continue to work after this patch. This patch includes API changes and zero modification in JS files.
If 1/ and 2/ are not reproducible (but I think we need more confirmation), then with those nitpicks fixed, the patch is good to go.
Comment #6
saltednutI am still seeing errors 1 & 2 from my earlier posts, but this may have something to do with Features. If no one else can reproduce, I assume its environmental.
I've attached a new patch that should fix the nitpicks from #4.
Comment #7
jlyon CreditAttribution: jlyon commented@brantwynn:
I am also experiencing the same issue as your #1 in comment #3. Both the Insert Media and Insert Drupal Teaser Break buttons are not showing up on the CKEditor profile edit page on the Appearance drag and drop. I have been able to get around this by inspecing the title of that field and disabling the
display: none
css code, but it's definitely annoying. I did not see any JS or PHP errors anywhere.I am not able to reproduce the issues in your #2 comment after upgrading to the latest media-dev with #2164831: Fix for CKEditor ACF (Advanced Content Filtering) configuration
Comment #8
jlyon CreditAttribution: jlyon commentedI am also experiencing a similar minor bug where CKEditor seems to remove the
<mediawrapper>
element wrapping the {fid} code.Steps to reproduce:
1. Insert a picture
2. Click on the picture. The path at the bottom of the image includes a mediawrapper element.
3. Click on view source, then go back to the WYSIWYG.
4. Click on the image again. The path at the bottom of the image no longer includes the mediawrapper element.
I have all three patches from das-peter's comment in #4. I have tried with ACF turned both on (with #2164831: Fix for CKEditor ACF (Advanced Content Filtering) configuration) and off.
I believe a similar issue is at play if you try to link an image that was inserted via media. Steps to reproduce:
1. Insert a picture
2. Click on the picture. Click on the link button, add a link.
3. Click view source. The mediawrapper element is gone, as is the
a
element.4. Go back into wysiwyg, create a new link, view source, the link appears.
4+. Similarly, if you view source between inserting the media and clicking the link button, the link will work.
It appears that the presence of the
mediawrapper
element in this instance breaks the link button.Comment #9
philipz CreditAttribution: philipz commentedI see no errors. Patch #6 works fine. I've tested adding and uploading files in CKEditor.
Comment #10
PI_Ron CreditAttribution: PI_Ron commentedI've tried the following and can't get the media embed button to appear in ckeditor profile settings:
Thanks for all your work on this and apologies if I am missing something simple here!
Comment #11
saltednutRe #10 - I don't think you're doing anything wrong.
I assume you have a custom appearance, no? That is to say, you've configured the buttons in a specific order?
From my experience using this patch, the button only shows up if the button configuration is left default - OR - one conducts this work-around.
1. Install Features - you should be using it anyway.
2. Export your CKEditor profile into a Feature
3. Add the [media] button to the profile manually.
Here is a link to an example Features export that does this. It is the only way that I am able to get the Media button to show up in custom configurations.
-- http://drupalcode.org/project/curate.git/blob/refs/heads/7.x-1.x:/curate...
Comment #12
jlyon CreditAttribution: jlyon commentedNote: To fix the issues with the media icon not appearing in the admin ui (actually any third party icon not appearing, including the Drupal Teaser Break, and the ), you need to apply the patch at http://drupal.org/files/issues/ckeditor-remove-external-plugin-declarati... from #2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working).
I could re-roll this patch with that applied, but that doesn't seem like a wise idea. I have the following patches applied and everything is now working for me:
- http://drupal.org/files/issues/ckeditor-accomodate-latest-media-changes-...
- http://drupal.org/files/issues/ckeditor-remove-external-plugin-declarati...
- http://drupal.org/files/issues/ckeditor-hook_into_media_admin-1649464-81...
Comment #13
saltednutHi @jlyon - I actually wrote the patch in #2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working) and we are currently using it - but still experiencing the bug.
So... I don't think that patch fixes the bug. Perhaps its something else about your configuration that's fixing this error.
Comment #14
PI_Ron CreditAttribution: PI_Ron commentedOk without changing any of the default button orders under appearance the button appears when using wysiwyg. I'm getting the following error though at the third step of media button process:
**Edit** Forget this, I just had to enabled media_wysiwyg module
Comment #15
das-peter CreditAttribution: das-peter commentedI just came across a case in which no media button was displayed.
It really looks like we've quite a mess with the declaration of the ckeditor media plugin (or the ckeditor plugin definitions at all).
The patch
media-ckeditor-button-poc.patch
shows the minimal changes I had to do to get the button.The patch
media-ckeditor-plugin-definition-fix.patch
is the approach I would prefer to clean this mess.Btw. features has indeed impact on the whole issue. It looks like "missing" plugin definitions are resurrected by
hook_ckeditor_profile_defaults()
. This means even if the plugins defined in'loadPlugins' => array()
aren't available anymore they are added to the plugin list. That "somewhat" works because'loadPlugins' => array()
not only contains the name of the plugin to load but the whole definition of the plugin - what seems to be totally wrong in my eyes...Comment #18
deggertsen CreditAttribution: deggertsen commented@das-peter, your patches are for the media module, not ckeditor. So they should probably be posted in a separate issue over there. However, the second patch fixes the problem of the media button now showing up. So thanks for that!
media-ckeditor-plugin-definition-fix.patch
Comment #19
PI_Ron CreditAttribution: PI_Ron commented@deggertsen so do we apply that patch to latest media dev?
Comment #20
das-peter CreditAttribution: das-peter commented@deggertsen thanks for letting me know that the patch helps - now it's worth to open a dedicated media issue.
I've found another issue with the plugin button handling in ckeditor itself. It has a regex pattern to detected buttons in the plugin code, but unfortunately it doesn't seem to work always.
The attached patch extends the patch from #6 with an fixed pattern. Check the interdiff to see what I've changed - it's just a minimal change to ensure it properly works with multi-line styling of the js code.
This change might fix the media button issue without patching media itself, however I still think we should patch media to ensure media_wysiwyg is taking absolute control if present.
Comment #22
das-peter CreditAttribution: das-peter commentedAs requested by #18 here's the follow-up for media: #2173911: Ensure media wysiwyg takes over control of ckeditor media integration
Comment #23
das-peter CreditAttribution: das-peter commentedOuch, looks like somehow the line-endings in the patch were messed up. Fixed patch.
Comment #24
das-peter CreditAttribution: das-peter commentedComment #25
mikgreen CreditAttribution: mikgreen commentedCan you please verify this bug. We encountered this on a project that uses latest madia module and this patch.
1. Select some text in Ckeditor.
2. Click "Add/Remove Numbered List" or "Add/Remove Bulleted List" button to make a list.
3. Click "Source" button to view html.
4. Click "Source" button to get back.
Result:
1. All content in editor field is gone.
2. All buttons in editor are grayed out.
3. Console shows error: "Uncaught RangeError: Maximum call stack size exceeded ckeditor.js"
I'd appeciate if someone could test if this is happening with your setup as well. Thanks!
Comment #26
kevinchampion CreditAttribution: kevinchampion commentedI can confirm the problem in #8. Every time the editor binds/unbinds the mediawrapper is stripped. I was hoping that part of this refactor was meant to address this problem and make it easier to deal with. However, is this issue now likely due to the media side of things? It looks like the relevant code might be in the media_wysiwyg module now.
Comment #27
das-peter CreditAttribution: das-peter commented@mikgreen / @kevinchampion Please specify the exact CKEditor version as well as the enabled ckeditor plugins (especially if the widgets plugin is enabled). Otherwise can be very hard to reproduce issues since we're dealing with a version set from 3.6 up to 4.1.x.
Comment #28
saltednutI believe turning off CKEditor's ACF provides a workaround for the issue described in #8 - I have seen the issue but not when the Advanced Content Filter is off.
Comment #29
mikgreen CreditAttribution: mikgreen commentedFor #25 I tried CKEditor versions 4.3 and 4.3.1. Happens in both versions.
Also: The browser is Chrome 32.0.1700.77.
Haven't tried with anything else.
I'd appreciate if someone would check this out and try to reproduce it. Maybe I'm doing something wrong.
Comment #30
kevinchampion CreditAttribution: kevinchampion commentedI can confirm #8 with the following ckeditor configuration:
https://dl.dropboxusercontent.com/u/57694/Screenshots/ckeditor_config.png
and the following module versions:
https://gist.github.com/kevinchampion/8514861
I've also tried downloading a custom version of ckeditor library with the widget plugin included in the bundle and get the same type of results.
Inspecting the javascript loaded on the page and plugin.js from media_wysiwyg isn't loaded on the page. Is it supposed to be?Update: nevermind, plugin.js is there.Comment #31
kevinchampion CreditAttribution: kevinchampion commentedCreated a new issue and patch to deal with issues I've seen that correspond with #8:
https://drupal.org/node/2177893
Comment #32
saltednutQuick question, has anyone tested #23? Also, @das-peter, does #23 address the missing button issue?
Comment #33
ayalon CreditAttribution: ayalon commentedThe patch #23 does not work at all. media_wysiwyg_include_browser_js() is called to late in the function ckeditor_profile_settings_compile().
The javascript settings which should be added to the page whitin the function media_wysiwyg_include_browser_js() do not appear in the source code and therefore the module is not saving overridden attributes and so on.
Comment #34
hefox CreditAttribution: hefox commented**
Why are these two modules not doing their own hook_ckeditor_plugin?
This isn't really correct usage of t function
!module_exists is more common style wise than == FALSE
Comment #35
saltednutRe: 34.2, that issue is addressed in #2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working) which is an accompaniment to this patch. (To be applied after, hence the failing test on it)
Edit: I think a Meta issue is in order. #2217579: [Meta] Getting this Module working with latest Media 2.x-dev
Comment #36
saltednutComment #37
rooby CreditAttribution: rooby commentedPatch (#23) no longer applies. Needs re-roll.
Comment #38
rooby CreditAttribution: rooby commentedAlso seems like this could be a bug and higher priority if media integration is broken.
Comment #39
iKb CreditAttribution: iKb commentedPatch based on the latest dev.
Comment #40
iKb CreditAttribution: iKb commentedComment #41
rooby CreditAttribution: rooby commentedAwesome thanks, it's working now.
I will report back if I find any bugs.
Comment #42
jcisio CreditAttribution: jcisio commentedNice work! I review again the patch and it looks good. I think we just need to wait a few days for feedback and I'll commit it.
Also, #39 did not address #34, could it be fixed? I know it is basically code move (#34.3) however it does not hurt to fix.
Comment #43
brockfanning CreditAttribution: brockfanning commentedI don't know specifically what the patch fixes, but I was getting javascript errors when I clicked the final "Submit" button to insert Media-module media into Ckeditor, and after applying this patch I'm no longer getting the errors. So, I am grateful, thanks! :)
Comment #44
joelpittetHere is a reroll (they removed the EOF ?> already on ckeditor.api.php).
Plus the changes in #39 and the addressing review from @hefox in #34.
1) May cause conflicts but did it anyways...
2) Don't know the answer to this one... maybe you can post what this should be?
3) Fixed.
4) Agreed and fixed.
Comment #45
joelpittetTestbot rollout!
Comment #46
rooby CreditAttribution: rooby commentedYou can ignore point 2. See #35.
Comment #47
wwalc CreditAttribution: wwalc commented@all - thanks a lot for your hard work on this patch. I’m sure it wasn’t easy to work with ugly legacy code in many places and with no tests.
I’m not going to give a full review of this as I have very little knowledge about the Media module, however I just wanted to point out some mistakes that I noticed in the latest patch.
1. The code in ckeditor.ckeditor.inc (ckeditor_ckeditor_plugin()) does not include the latest changes introduced in CKEditor module to support loading CKEditor from a remote URL (and the official CDN)
For example:
has been replaced with just:
You can easily verify this by downloading the latest dev version of CKEditor module. The path to CKEditor is set to a remote CDN, to a package with all available plugins. The tableresize or autogrow plugins may be enabled on demand in the CKEditor profile. After applying the patch and making sure that Drupal is using the updated code, it is no longer possible.
Before:
After:
2. The code for registering “CKEditor module plugins” and “CKEditor module plugins - additional directory” has been simplified without any explanation why was it done. Basically, the code that tries to find the buttons (icons) provided by a plugin has been removed.
has been replaced with:
The main idea behind this feature was to allow users to simply unpack additional JavaScript plugins to the “plugins” folder in the CKEditor module directory and be able to enable them in the toolbar without writing any additional module.
You may test this by using the current version of CKEditor module, and unpacking the youtube plugin in sites/all/modules/ckeditor/plugins folder. The button will magically appear then in the CKEditor administration area in the toolbar configuration tool.
Actually, you don’t have to download this additional plugin, because you can test it as well using e.g. imce/mediaembed plugins that are in this folder (for imce, you need to install that module first). You can see that these additional buttons are gone after applying the patch.
Before:
After:
Considering that this feature has been already for ages and might be used by many existing instances, I would not remove it. It should not conflict in any way with the Media support which is introduced here and in general I believe that such changes in module behavior should be handled in separate issues, to have a clean track of why eventually something has been dropped.
I hope you agree with me and I keep my fingers crossed!
Comment #48
Devin Carlson CreditAttribution: Devin Carlson commented@wwalc, thanks for the feedback!
An updated version of the original patch which addresses the points from #47.
The patch is mostly moving code around and the automatic parsing of plugins is left alone with the exception of the final check for plugin.js and buttons. I believe the final check is redundant since plugins in the ckeditor library are already detected and plugins provided by modules already have to declare their buttons. The latter can lead to tricky problems such as the hook implementation specifying an incorrect path but the plugin buttons are still displaying because of the auto detection.
I also haven't cleaned up any existing plugin definitions, as much of them will go away with #2158741: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working).
Comment #49
martindilling CreditAttribution: martindilling commented48: ckeditor-accomodate-latest-media-changes-48.patch queued for re-testing.
Comment #51
martindilling CreditAttribution: martindilling commented48: ckeditor-accomodate-latest-media-changes-48.patch queued for re-testing.
Comment #53
Devin Carlson CreditAttribution: Devin Carlson commentedA reroll of #48.
Comment #54
hefox CreditAttribution: hefox commentedIs this correct, '/'? shouldn't it be base_path()?
Comment #55
agoradesign CreditAttribution: agoradesign commentedToday I wanted to try CKEditor module instead of WYSIWYG, but unfortunately had the problem, that media tokens are immediately converted to img tags.
I've posted more info here in this thread: https://drupal.org/node/2126755#comment-8758547
Comment #56
brockfanning CreditAttribution: brockfanning commentedPlease forgive if this is not the appropriate place/procedure for this. This (much appreciated) patch seems to be all about making CKeditor and Media play nice together, and although it gets me almost there, I'm still getting javascript errors when I try to embed non-image media, such as audio files. The attached change fixes my problem, but I'm a media/ckeditor newbie, so I hope someone with more experience can review. Thanks.
Comment #57
saltednut#53 is working for me in every scenario except for when I am doing inline editing using http://drupal.org/project/edit <-- Backport from D8.
In that scenario, the Media button shows up, but nothing happens when I click on it.
I think that's a total edge case though, and should be a followup.
Comment #58
jcisio CreditAttribution: jcisio commentedChange the issue title to make it clear. Otherwise we won't finish.
As described in #2, what needs to be tested (before and after) is:
1. CKEditor admin UI still works (plugin settings)
2. Features integration still works (features update/revert, no feature change before and after the patch).
3. Front end still works: basic buttons and CKEditor behavior.
Then it should be OK to go. The patch is rolled with that idea in mind, thus I keep the issus status as "needs review".
DON'T test this patch with another patch and another patch and another moving patch.
Comment #59
PI_Ron CreditAttribution: PI_Ron commented@brantwynn Do you know of any way to have media button working with edit module?
Comment #60
saltednutRe #59 - I have had mixed results, actually. It seems to function since updating my profiles recently and using the latest Edit dev.
Comment #61
brockfanning CreditAttribution: brockfanning commentedGoing to hide my patch, since it's not really related to what's described in #58.
Comment #62
Angry Dan CreditAttribution: Angry Dan commentedI think this issue's description needs to be updated to reflect what the patch actually does (and I mean everything), to cover off motivations and to be clear on what's left to do.
Referring to #58/#2, as far as I can tell:
> 1. CKEditor admin UI still works (plugin settings)
Works for me.
> 2. Features integration still works (features update/revert, no feature change before and after the patch).
Features integration does still work and there is no diff in the feature, although that is a problem as I'll explain in a moment.
> 3. Front end still works: basic buttons and CKEditor behaviour.
Not quite. If the old CKEditor media plugin was enabled before applying the patch, then it won't work afterwards. This is because we're actually storing a reference to the plugin path in the database. The plugin is now duplicated which isn't compatible with module_invoke_all(), but even if you remove the plugins/media folder the database still references the now missing folder.
So, the attached patch does two things: a) Remove the now redundant media plugin from this module, and b) provide an update hook to fix the invalid path issue. Which brings me back to point 2 - features - we have to fix the references in the database, and therefore we have to mark the feature overridden.
Comment #64
moonray CreditAttribution: moonray commentedThe patch in #62 removes plugins/media/library.js but it's still being referenced in ckeditor_profile_settings_compile() (in ckeditor/includes/ckeditor.lib.inc ).
Seems things were missed.
Since I'm not entirely sure of the why and how, I'm not able to update the patch properly.
Comment #65
Angry Dan CreditAttribution: Angry Dan commentedAs far as I can tell that reference is no longer necessary, and the attached patch removes it.
I'm actually a little confused by the code in
ckeditor_profile_settings_compile()
:Is it not possible for media_wysiwyg to do this for us? Why do we have to do it? We're introducing a cross dependency here that seems unnecessary. I'll do some digging, but it's possible that CKEditor is going to have to provide some kind of hook to each module that's providing an active plugin, or some other method of allowing them to including their ancillary files.
As far as I can tell, this code is the legacy stuff tied to the older media browser we had in CKEditor before now, so I'm removing it given that I've removed the rest of CKEditor's built in support for the media module.
If I'm honest I don't think that the CKEditor module's media functionality has ever worked properly anyway, so stripping it all out is probably for the best.
Comment #66
Angry Dan CreditAttribution: Angry Dan commentedComment #68
Angry Dan CreditAttribution: Angry Dan commentedSo just realised this should have been a --binary patch; otherwise it's identical to #65.
Comment #69
drclaw CreditAttribution: drclaw commentedHi,
Tested out the patch today. As far as I can tell it's working as expected, but there are a couple quirks using it on a site that already has other ckeditor plugins installed. Any existing hook_ckeditor_plugins implementations will need to have the path updated to include the base_path, otherwise it will be made relative to the ckeditor library directory. If you're getting errors that look something like the following:
...you'll need patch the module providing the plugin HOWEVER, after fixing the path, you'll need to save the ckeditor settings page again. I also needed to clear my browser's file cache as it was caching some of the ckeditor configuration. And you know... maybe clear Drupal's cache as well... for good measure...
On a related note to the above, I think Comment #54 is still valid. The ckeditor.api.php example has a hard coded
/
but should probably bebase_path()
... unless there's some reason I'm unaware of?Comment #70
tancI concur with drclaw's findings. Media embedding works as it should from my quick tests, with the token being used rather than the img tag. The linkit (7.x-3.x-dev) module was causing ckeditor's javascript to break but disabling that module allowed ckeditor to load.
Comment #71
agoradesign CreditAttribution: agoradesign commentedI've just tested the Media integration on a clean install, with the following setup:
* Drupal 7.30
* File Entity 7.x-2.0-alpha3+32-dev (2014-Jul-31)
* Media 7.x-2.0-alpha3+100-dev (2014-Jul-29) including media_wysiwyg sub module
* CKEditor (module) 1.15 with the patch from #68
* CKEditor library 4.4.3 (15 Jul 2014) - full package
active text filters (weighted order):
* filter_html
* media_filter
* invisimail_encode_html_entities (from Invisimail module)
* filter_url
I can confirm, that it also works for me now! :-))
Without the patch from #68, it wouldn't work.
Only disadvantage, I've experienced: it seems, that the patch breaks the integration with CKEditor LInk module - which sounds similar to the problem that tanc described with linkit integration.
Comment #72
Angry Dan CreditAttribution: Angry Dan commentedWe should probably do some brief discovery on that linkit/link module bug, to see what the problem is and how easy it is to fix, but given that the current state of the wysiwyg/media plugin is that it's essentially broken I think we should move towards a commit sooner rather than later?
I don't know if this module has a release schedule but assuming that the change won't go out as a stable release immediately, having the -dev exposure or creating a beta release will help us unearth any of the (inevitable) issues that such a large patch is likely to throw up?
Comment #73
tancSounds extremely sensible to me. I'll try and look at the linkit issue early next week as its part of our company install profile so would be useful to fix.
Comment #74
agoradesign CreditAttribution: agoradesign commentedI know the problem, and I have found a solution!
It seems, that the patch not only addresses the media plugin integration, but also tries to clean up generally the way, how plugins are loaded. Before, the ckeditor_load_plugins() function was 292 lines long: first, several plugins were defined on behalf of other modules, e.g. CKEditor Link and LinkIt. Later on, the 'ckeditor_plugin' hook was called, giving other modules the chance to define a CKE plugin. These hook implementations were merged with the array of plugins defined before.
It was a good idea, that this patch reduced complexity dramatically. The ckeditor_load_plugins() function is now only 13 lines long! Because all the plugins that CKE defines on behalf of other modules, are now being done in a self-implementation of the 'ckeditor_plugin' hook (function ckeditor_ckeditor_plugin() in the new ckeditor.ckeditor.inc file). This is best practice, like it should be done, and practiced by many other modules. All this implementation stuff was just moved to another place.
Normally, this shouldn't cause any problems. But the devil is in the detail: first of all, CKEditor Link already implements this hook by itself. But the way, how module_invoke_all() handles the duplicate plugins is different from an array_merge() call: you would have arrays for name and path instead of a single string now. That caused problem number one.
If we would just throw out CKEditor's on-behalf implementation, we would run into the next problem: the "default" key is not set (not sure, if it's needed or not), and the path is wrong. CKEditor would treat it relative to the library path. That's why, it prepends the path with '%base_path%' in its on-behalf implementations. As far as I have seen in the Code repository of LinkIt, we have exactly the same situation over there.
As I have to leave in a couple of minutes, I can't provide a patch until tomorrow, but I leave you a detailed solution here:
1. Go to ckeditor.ckeditor.inc file and drop lines 32 to 55 from ckeditor_ckeditor_plugin() function
2. In the same file, go to the end of the ckeditor_ckeditor_plugin_alter() function and add the following lines:
Step 2 is only a workaround, we should contact both maintainers to adapt their hook implementations, so that we don't have to do the code alteration.
Have a nice evening!
Comment #75
agoradesign CreditAttribution: agoradesign commentedI've updated Angry Dan's patch from #68 and moved all the stuff relating external plugins from other modules (ckeditor_link, linkit and ckeditor_swf) from hook_ckeditor_plugin() to hook_ckeditor_plugin_alter(), adding 'default' key and adapting the path.
I've done some quick tests. Media integration still works, CKEditor Link also works for me. However, I did neither try ckeditor_swf nor linkit. But normally there shouldn't be any problems.
I totally agree with Angry Dan, that we should get this committed asap. The media integration is very important. Additionally, cleaning up the whole plugin system is essential! And if this clean new approach to the plugin system has some unwanted side effects on existing plugins of other modules, than we have to accept that consequences and open up a new issue, or e.g. discuss it in this one: External plugin declarations are redundant. (Linkit, ckeditor_link, ckeditor_swf not working) (which hopefully has become obsolete now)
Comment #77
rooby CreditAttribution: rooby commentedI'm also plus one for committing this.
I've been using it for a while on a few sites and it seems to be fine (100% better than without the patch) and it would be good for people to not have to keep rerolling this all the time.
Comment #78
Angry Dan CreditAttribution: Angry Dan commented@agoradesign if you could provide an interdiff for your patch it would help us review your changes, it's easier than it looks and saves time when we're browsing the queue - https://www.drupal.org/documentation/git/interdiff.
If we're agreed that we should be moving towards commit, then let's push ahead now for RTBC. I'd love to test the patch in #78 myself but I'm going to be away from Drupal until the end of August.
Are there any active module commiters listening in on this issue? It's really important that we get your backing on how close you think this is to commit, even if the answer is "nowhere near".
Thanks all,
Dan
Comment #79
jcisio CreditAttribution: jcisio commentedI'm actively following this issue, and even gave feedback a few times. However, I don't have a consistent feedback: many people report error or incompatibility. Just a few "it works for me" without neither review nor modules that you are using (that relate to this patch i.e. the modules that provide plugins). Nor without saying if the export features changes. Nor without saying if the admin UI changes.
This issue is to clean up (rearrangement of plugins). Please review and RTBC. I haven't seen anyone doing it. The issue is always "needs work". The patch is not adjusted to take into account of comments (like #54, #69, #70).
PS: I haven't reviewed the patch since a while. I'll do that when the patch is RTBC and I have some free time.
Comment #80
Angry Dan CreditAttribution: Angry Dan commentedThanks for your comments jcisio. Can somebody please update the issue summary with a current status report (there's a template somewhere isn't there?) so that new visitors are able to help with the testing?
Comment #81
agoradesign CreditAttribution: agoradesign commentedHere's the interdiff, plus I've uploaded the patch again, hopefully satisfy the testbot now (binary patch)
Comment #82
heddnAdded tag to update issue summary. Its really not clear what this issue is going to fix or what functionality it will provide.
Comment #83
bkosborneCan someone more familiar with this issue update the issue summary to reflect what's being changed? Right now the title of the issue and the summary don't really match.
I noticed a few issues with the CKEditor integration, but they are likely unrelated to this issue:
- Entering alt or title text for an existing image doesn't actually do anything - the data isn't put into the media token
- When re-opening the media dialog for an image that was already embedded, the previously selected view mode is no longer selected and must be reselected. It always goes back to "Default" for me when I had selected a different one when embedding it.
Comment #84
saltednutI have tested #81 extensively and it is working correctly. Here is the recipe I used to test.
CKEditor plugin config: http://cgit.drupalcode.org/curate/tree/curate.features.ckeditor_profile....
I think we should move forward here. There is always room to follow up with meta issues if there are lingering problems. This is a big step forward for Drupal in the media space where CKEditor integration is largely accepted as patently broken.
Comment #85
micbar CreditAttribution: micbar commentedThe goal of the patch in #81 is to move the plugin definitions to a new file ckeditor.ckeditor.inc and to document the new hook_ckeditor_plugins_alter in ckeditor.api.php.
Additionally, this patch removes the old media plugin which seems to be a duplicate of #2140155: Remove media plugin. Media handling has been transferred to media_wysiwyg module a long time ago in #1504696: Integration with CKEditor module.
I set this to needs work because the patch misses some new plugins. it removes
from ckeditor.lib.inc
and doesn't add it to ckeditor.ckeditor.inc.
I try to provide an updated patch soon.
Comment #86
micbar CreditAttribution: micbar commentedComment #87
saltednutOne goal of the patch is to more easily enable other modules to add their own plugins. However, CKEditor also provides its own plugins. We should remove any plugins that are module dependent but keep the ones that are internal.
Comment #88
saltednutComment #89
micbar CreditAttribution: micbar commentedI made the suggested changes from #85. See interdiff.txt
Comment #91
micbar CreditAttribution: micbar commentedMy fault, forgot the --binary option.
Comment #93
micbar CreditAttribution: micbar commentedCould somebody review this patch, that we can move further with the follow-ups?
It has already been on RTBC (see #84). I made minor fixes in #89. See interdiff.
Comment #94
MediaFormat CreditAttribution: MediaFormat commentedn/a
Comment #95
wwalc CreditAttribution: wwalc commentedThanks to all of you for working hard on this issue.
@micbar - I saw your previous comment in #2159403-85: Make CKEditor plugin system modular and clean where you noticed missing CKEditor plugins, this was one of the reappearing problems and I was happy to see that you did not miss it.
I'll thoroughly check your patch next week, in the meantime if anyone wants to add any feedback regarding the latest patch, for example "seems to work" if you are affraid of saying that "it's perfect" and RTBC, then please feel free to do so.
Comment #96
tancI've tested #91 and am happily using it on a development site at the moment. Will report back if I notice anything unusual.
Comment #97
jcisio CreditAttribution: jcisio commentedWe needs someone using modules like Linkit, CKEditor Link, Media (1.x) to report what happens before/after the patch, for which version of each module. Removing of the integration with these modules is out of scope of this issue.
Ideally, the patch should only move thing around and add examples. We aren't "fixing" anything here, other than make the plugin system extensible.
Comment #98
rooby CreditAttribution: rooby commentedI use linkit with media 2.x and have had no problems.
ckeditor link and media 1.x I haven't tried.
Comment #99
wwalc CreditAttribution: wwalc commentedAlright, that was fun! Congrats to all of you, we're close to RTBC, there are just some tiny details to be cleaned up.
At the same time apologies for the delay, I was hoping that reviewing will take me less time.
Plugin paths
1) I did check the patch together with a couple of available modules that integrate with CKEditor using hook_ckeditor_plugin():
2) I tested CKEditor loaded from sites/all/libraries and from CDN.
It turned out that the "hack" in ckeditor_ckeditor_plugin_alter() for three plugins which were previously hardcoded is wrong, because it targets just these three plugins while all the external modules turned out to be broken. This is not surprising, because the old code did the following:
So the old code assumed that $base_path isn't provided, while the new one suggests doing it. I'm quite fine with that, but to maintain backwards compatibility we still have to add base_path() to plugins registered in the "old way". I did this by checking whether the path starts with "%" (indicates a placeholder like %base_path%), http://, https://, // or "/" - in all other cases I still append base_path(). Hence the new patch.
ckeditor_update_7006
I have doubts regarding this code in ckeditor.install. The old plugin should be removed from profiles automatically - I agree, but can we really assume that anyone who had the old plugin enabled can instantly enable media_wysiwyg module which is available only in version 2.x (alpha)?
For me such suggestion should be available in a changelog / release notes.
I did keep this in the patch, but would appreciate any additional feedback on this.
Notes (ignore)
In case I was wondering some day about why this part of code was gone, there were at least 4 reasons for this:
TL;DR
The attached patch fixes resolving paths for external modules. I'm pretty convinced that it works, but a double check would not hurt.
I'm for modifying ckeditor_update_7006() because of the things I wrote above, but maybe I'm missing something.
Comment #101
wwalc CreditAttribution: wwalc commentedAnother attempt, with --binary this time ;-)
Comment #102
wwalc CreditAttribution: wwalc commentedComment #103
rooby CreditAttribution: rooby commentedIn relation to ckeditor_update_7006, if you want to go the message way there could be an update function that displays a message to the user on screen also.
Comment #104
rooby CreditAttribution: rooby commentedOr we could also detect their version of the media module and act accordingly.
One thing I'm not sure about with that approach would be is it possible a person upgrades from media 1.x to 2.x at the same time as this update and we detect media 1.x before it has been updated to 2.x and then it updates to 2.x.
I can't remember exactly how the upgrade process runs so I'm not sure if that kind of thing would be an issue.
Plus that's getting pretty complicated and probably doesn't need to be.
Comment #105
joelpittetMinor nitpicks, I don't actually follow what this issue is doing... but looks like it's cleaning things up so good on you:)
Should be inline comments for coding standards.
Remove debug code.
Space, Captital, period.
Comment #106
dasjoAlso see #2352573: CKEditor Media WYSIWYG library.js does not pass entity fields into create_element
Comment #107
wwalc CreditAttribution: wwalc commentedThe updated patch contains the following changes comparing to ckeditor_plugin_system_cleanup-2159403-100.patch:
Any feedback is welcome!
I'd love to commit this soon to have more people testing it. After having the security release published last week, finally we can go ahead and fill the dev version with changes like this one, that were waiting for a while.
Comment #108
deggertsen CreditAttribution: deggertsen commentedJust spent some good time testing and so far I haven't run into any problems! So excited to have this finally fixed!
Comment #109
marcusx CreditAttribution: marcusx commentedIt works fine with images. But can someone confirm it works with PDFs / Documents? See: #2364559: Link / Icon to media document is stripped out by ckeditor before save or when switching to markup view
Comment #110
Devin Carlson CreditAttribution: Devin Carlson commented@marcusx that issue is on the Media side.
+1 on RTBC and committing #107. I haven't run into any issues with it during my testing as well.
Comment #111
partdigital CreditAttribution: partdigital commentedI can confirm that this works with Media and Media Youtube as well.
Comment #112
marcusx CreditAttribution: marcusx commented@devin-carlson: Really? Which issue is that?
I wonder because I have content with media tokens added with an older version / combination of media / ckeditor.
e.g.
This media token is correctly handled as link with PDF icon no matter what I do. I can switch to "Source" in ckeditor. Or change to "Plain Text". It is not breaking. But when adding a new PDF from the media library I don't get the correct stuff embedded.
Comment #113
BrightBoldWith the patch in #107 and the "Plugin for inserting embeded [sic] media" checked, my editor window failed to load - I just get a white area the size of the editor window, and the page loads scrolled to the text format details below the editor window. I reproduced this in both Chrome and Firefox under the following conditions:
If I either uncheck the embedded media plugin or roll back this patch, the editor appears normally. I found this issue but it's for the WYSIWYG Media Embed module (which I'm not using).
I didn't want to set this back to needs work without asking if there's a configuration issue I should fix or whether this issue is really related to something else. If nothing else I'll save someone else hours trying to figure out why their editor window disappeared.
Comment #114
wwalc CreditAttribution: wwalc commented@BrightBold - could you provide more details, for example all the JavaScript exceptions in the error console?
Make sure to clear the browser cache and perhaps check two different browsers.
Comment #115
saltednut@BrightBold - Media is currently adding an extra checkbox in the settings. Maybe that is where your problem lies?
A couple screenshots of what I have working in D7 with Media 2.x-dev, File Entity 2.x-dev, CKEditor HEAD (w/ patch).
Comment #116
BrightBold@brantwynn - Yes, it definitely turned out to be an interaction between that extra checkbox, "Plugin for inserting embeded media" and this patch. Either one by itself was OK, but both together messed up the script.
@wwalc - I will redo my testing and provide JavaScript console info in the next few days.
Comment #117
saltednut@BrightBold brings up a good point here. Should Media be providing the checkbox or should CKEditor? I believe right now they are both providing a checkbox.
Comment #118
BrightBold@brantwynn From an architectural perspective I don't know which module should provide the link, but I will point out that the way they're written now, it's not clear that they perform the same function. I assumed that "plugin for inserting images" only inserted images, and that I needed the embedded media plugin to get video working, which was the problem I was trying to fix.
So the wording of "embedded media" is better since it more accurately reflects the button's function. But if that wording wins, we should be sure to spell "embedded" properly.
Comment #119
wwalc CreditAttribution: wwalc commented"Plugin for inserting embeded media" has nothing to do with the Media module. It is a separate plugin which allows you to paste
<iframe>
from youtube services etc. It has been in CKEditor module for ages and it's really... "basic" let's say. I would not introduce it today, but considering that some people might be using it it's better to leave it and just discourage from using it.Words "Embedding" and "Media" have such a broad meaning that indeed the naming is a bit confusing.
Let's just wait for BrightBold to confirm that the bug was indeed caused by conflict with ""Plugin for inserting embeded media" (or by that plugin being just broken) and we're ready to go. The issue can be then handled in a separate ticket (as it touches an unrelated plugin), in which we could also decide about marking that separate plugin as "deprecated" or sth so that people did not enable it accidentally.
Comment #120
saltednutA fast grep (ack) for 'embeded' turns up
/ckeditor/plugins/mediaembed/plugin.js
I am unsure if its in the scope of this patch to remove the 'embeded' media plugin within CKEditor module, but it definitely appears that we'd want the module providing the plugin (e.g. Media) to be the one providing the button.
Comment #121
wwalc CreditAttribution: wwalc commented@BrightBold - if you confirm that the issue is caused by the plugin with the typo in its name ("Plugin for inserting embeded media"), please post the errors from the developer console anyway so that I could investigate this bug.
Comment #122
saltednutCross posted with #119 - thanks for the clarification @wwalc
Comment #123
rooby CreditAttribution: rooby commentedI think removing the mediaembed plugin is outside the scope of this issue.
Comment #124
BrightBold@brantwynn reminded me I'm holding up this issue - so sorry! I got caught up with a site launch. I will do some more testing this evening to provide the requested JavaScript exceptions. Is there any other information that I can give you at the same time that will be helpful?
Comment #125
BrightBoldAargh. Well now I can't reproduce it. So either it was a conflict with something in Media alpha4 or File Entity beta1 and I didn't clear the cache when I updated those to the dev versions, or it was a conflict with a Media patch that I later rolled back. So you should go ahead with this. Sorry to hold it up and then not be able to figure out what I changed to make it irreproducible.
Comment #126
saltednut@BrightBold Thanks for looking into it!
@wwalc is there anything else we need to do here?
Comment #127
jcisio CreditAttribution: jcisio commentedI tested patch 107 with Features export: global + advanced profiles (plugins: auto grow, code snippets, Scald dnd, Drupal teaser, MathJax, embeded media). The export before and after the patch were identical.
Comment #128
heyyo CreditAttribution: heyyo commentedNot sure what I'm doing wrong.
I could successfully input a youtube or vimeo inside the field "File URL or media resource *" which is accepting oembed provider.
After submitting my video, I could see it in Ckeditor correctly but as soon as I save the node or even if Switch to plain text editor, the embeded code is not there anymore.
PS: regular images works nicely.
I have the following installed:
file_entity-7.x-2.0-beta1
media-7.x-2.0-alpha4
media_oembed-7.x-2.2
ckeditor-7.x-1.16 + patch#107
ckeditor library: //cdn.ckeditor.com/4.4.3/full-all
drupal 7.34
Enabled:
ckeditor
file + image
media_internet
media_wysiwyg
media
media_oembed
Input filter in this order:
CKeditor
Plugin for inserting images from Drupal media module is checked and in the toolbar
Advanced content filter is disabled
Comment #129
jcisio CreditAttribution: jcisio commentedThe patch has been RTBC since more than a month so I was going to commit it. But when I look at it, the media plugin is removed. What happens? Does it mean that CKEditor does suddenly not support Media 1.x? Or if the plugin is broken, then since which version of Media 1.x?
I think that plugin should be kept (it has nothing to do with making the plugin system modular and clean). And if we don't want it in Media 2.x, that module implements hook_ckeditor_plugin_alter() to replace it with its own version.
Comment #130
jcisio CreditAttribution: jcisio commentedBTW the issue summary needs update. We have tags, but I think a comment is necessary.
Comment #131
heyyo CreditAttribution: heyyo commented@jcisio any idea on my issue #128 ?
Comment #132
jcisio CreditAttribution: jcisio commented#131 It's out of scope. Only thing that works before the patch and does not work after is in the scope of a refactoring.
Comment #133
msmithcti CreditAttribution: msmithcti commentedFor those (@heyyo and @marcusx) concerned about how this works with media other than images, I found I was running into the issue described in #2364559: Link / Icon to media document is stripped out by ckeditor before save or when switching to markup view and have just posted a patch that fixes the issues for me, alongside the patch from this issue.
Comment #134
Devin Carlson CreditAttribution: Devin Carlson commentedAn updated version of #107 which:
ckeditor_profile_settings_compile()
. An single additional check was added to verify that the JS is only added when Media 1.x is installed (Media 1.x implements the WYSIWYG text filter itself while Media 2.x moves the filter into the Media: WYSIWYG submodule).This leaves altering/removing the default plugin up to the Media module.
Comment #135
saltednutWith the standard profile, I am able to load and configure CKEditor and its profiles correctly for Media but I see this JS error when I try to use the Media button:
Within my install profile (Lightning) I am able to use CKEditor correctly, but I see a number of errors during install and also when I visit the CKEditor profiles pages.
Comment #136
saltednutMy apologies: It appears Media integration is now out of scope for this patch. Safely ignore #135 as it will be addressed in a followup in the Media module's queue.
Comment #137
saltednutTesting #134 inside the standard profile with CKEditor module shows all standard buttons operating correctly. See screenshot.
Comment #138
agoradesign CreditAttribution: agoradesign commentedSorry, I've to revert the status, The patch from #134 fully broke a site, where #107 works.
After experiencing some loading problems with CKE on a couple of sites, when having more than one CKE-enabled field, I decided to give the latest dev versions of File Entity, Media, CKEditor including the updated patch from #134 a try. I've tried to use it on a site that already had the patch from #107 applied.
First, I've experienced a couple of "Array to string conversion in ckeditor_ckeditor_plugin_alter()" errors, because both CKEditor and Media WYSIWYG register a "media" plugin. Because of that, the media plugin definition contains an array as path, not a string. Ok, this could be avoided with a simple if condition. But the next and bigger problem is, that Media WYSIWYG also implements the ckeditor_plugin_alter hook and runs after CKE. There the path is again modified. So the JS files provided by CKEditor module never get called. And therefore the editor won't load because of a "Drupal.media is undefined" JS error.
Is there a special reason for the re-inclusion of Media plugin in #134? Because in this case brantwynn's comment in #136 is wrong and Media integration is back in scope.
Comment #139
jcisio CreditAttribution: jcisio commented#138 This issue does not fix anything, it just reorganizes the plugin system. The media plugin is back, because it has been always there. But you are correct about the plugin alter hook. CKEditor should not use that hook and move the unset condition into the plugin hook itself, so that Media module can easily alter it.
Comment #140
agoradesign CreditAttribution: agoradesign commentedBut even if Media alters it, it goes wrong (the JS error I described). I'm not sure, but I think it is either because Media does alter the path and override CKEditor's Media plugin path, or the fact, that these three lines were dropped off between the patches:
If yes, what must Media WYSIWYG do, that these files get loaded and everything works as expected?
Comment #141
Devin Carlson CreditAttribution: Devin Carlson commentedAn updated patch which moves the code out of
ckeditor_ckeditor_plugin_alter()
and intockeditor_ckeditor_plugin()
(except for the compatibility shim which should exist inckeditor_load_plugins()
).I also added a few extra code comments.
Comment #142
agoradesign CreditAttribution: agoradesign commentedThanks for the updated patch. Looks good - no more PHP errors, and with latest Media dev also no more JS errors nor problems
Comment #143
saltednut#141 is working great for fresh installs of our distribution and even fixed #2331293: Quickedit module compatibility on my end. I'd say this is RTBC as long as @jcisio et al are happy with it.
Comment #144
DamienMcKenna+1
Comment #145
daften CreditAttribution: daften commentedFor me the patch doesn't work properly. By applying the patch and trying to use the media plugin, clicking the button gives the following error:
I know that the media plugin packaged is deprecated and should be replaced by enabling the media_wysiwyg module, but when I do that, the following errors pop up on the configuration page for the ckeditor profile:
This seems to be because the plugin with the same name is declared twice (once in ckeditor itself, once in media_wysiwyg). I don't see any way to fix this problem from the media_wysiwyg module's end.
Comment #146
aDarkling CreditAttribution: aDarkling commented+1
#141 made ckeditor actually useable for my purposes now! at
Now that media tags are being generated, I'm working out ckEditor/Media/Picture intergration.
That shouldn't stop this from getting commited, though. The issue queue's already kinda congested.
This patch also covers #2292575: "Naming of mediaembed plugin is confusing".
Comment #147
hass CreditAttribution: hass commentedThis is incorrect way to add links into translatable string as it is no context sensitive.
Same
Same
Same
Translatable string?
Same
Translatable string?
What are we translating here? This can be a security issue.
Translatable string bug. Requires placeholders.
This is far away from being RTBC.
Comment #148
jcisio CreditAttribution: jcisio commented@hass those are (or mostly are) just code moving around, so I guess we can live with that, and fix them in follow-ups. This issue has been hold for too long.
If there is no more functional problem, I'll try to review and commit it this weekend.
Comment #149
hass CreditAttribution: hass commentedThese should be fixed while we are already on this buggy code. It's not really difficult to fix.
Comment #150
hass CreditAttribution: hass commentedThese should be fixed while we are already on this buggy code. It's not really difficult to fix.
Comment #151
daften CreditAttribution: daften commentedThe latest version of media fixes the issue, this patch works perfectly for me now :)
Comment #152
dougn7 CreditAttribution: dougn7 commentedHi everyone. Im apply this patches on dev versions on Media (2.x) and CKEditor and still the last submit button doesnt work. Got some Hunk messages on Git, but the rest is ok. I put "config.allowedContent = true;" on both CKEditor profiles and such. Any help?
Comment #153
aDarkling CreditAttribution: aDarkling commentedRegarding comments in #147
1-4. Fixed
5. I Agree. Fixed.
6. Fixed
7. I Agree. Fixed.
8. You're right. Unless someone wants to go through the entire ckeditor/addons listing and pull out every description, t()'ing this would be a waste of time. Changed it to run through check_plain() so there won't be any security issues.
9. This should be translated to accommodate the right-to-left folks as well, so I just changed it to use a placeholder.
Included is the updated patch. I also added in support for the Image2 plugin that's required if you want to use ckeditor's captioning widget. See? Feature Bloat. Let's get this reviewed & out the door before it happens again! :)
Comment #154
jcisio CreditAttribution: jcisio commented1. Please post interdiff. The attached patch misses a file.
2. We should be careful with the coding standard corrections that modify strings. All modified strings will become untranslated in non English websites. They are mostly in the back office, so it could be acceptable, but needs review if we want to change them.
Comment #155
hass CreditAttribution: hass commentedTranslatable trings that are already broken cannot break more. Everyone is running l10n_update and will get the new translations asap.
Comment #156
jcisio CreditAttribution: jcisio commented#155 only if someone translated all new strings first...
We don't follow coding standard just for the sake of coding standard, if it breaks current websites for no good reasons.
Comment #157
daften CreditAttribution: daften commented#156 Yes, current websites might have English instead of a localized string, that is no reason to not follow coding standards which are there for many reasons, security as the main one.
What I'd propose is to either fix this here, or to create a follow-up issue with a patch which fixes this. But IMO it needs to be fixed at some point.
Comment #158
jcisio CreditAttribution: jcisio commentedSure, but it depends how they affect security. I can't say anything until I see the patch, which was missing in #153.
Comment #159
bneil CreditAttribution: bneil commentedHere's an interdiff between the patch in 141 and the one in 153.
Comment #160
hass CreditAttribution: hass commentedI have one question about this plugin hook. Is it possible for a theme to define the hook and implement per theme plugins? e.g. image2 is a very good candidate for this. Some themes may be compatible with image2 where others are not.
Comment #161
aDarkling CreditAttribution: aDarkling commentedCripes! So embarrassing!
Ignore #153. I forgot to git-add the only file that I worked on!
Here's a complete patch & associated interdiff.
Comment #163
hass CreditAttribution: hass commentedTwo spaces and a trailing space?
trailing space?
trailing space?
Comment #164
aDarkling CreditAttribution: aDarkling commentedSorry. Didn't know trailing spaces was a thing.
Here's one that works. Reverted & re-patched locally to be sure.
Comment #165
hass CreditAttribution: hass commentedStill two spaces in several strings.
Comment #166
aDarkling CreditAttribution: aDarkling commentedOK, this should do it.
Should those other posts get deleted?
Comment #167
rooby CreditAttribution: rooby commentedComment #168
rooby CreditAttribution: rooby commentedOops, missed some.
Comment #169
hass CreditAttribution: hass commentedStrings are fine now. Great.
Comment #170
bneil CreditAttribution: bneil commentedUpdated the issue summary based on variety of comments on this issue.
Comment #171
Devin Carlson CreditAttribution: Devin Carlson commentedI think this is back to RTBC per #169.
Comment #172
izmeez CreditAttribution: izmeez commentedThe patch in #169 applies cleanly. Not sure what is holding this patch back.
The media browser comes on and media can be selected. It initially appears correct in the wysiwyg editor as an icon and link but when it is saved it is saved as just text. This is an issue even without this patch. I'm still digging through the issues to find the correct issue for the problem. Didn't know if this patch was going to fix that bit too. [edit] Found #2177893: Custom wrapper breaks tokens with CKEditor >=4.0 without widget plugin includes patch that solves this problem. [/edit]
Comment #173
rooby CreditAttribution: rooby commentedI hope I'm wrong about this because I've been waiting for this to be committed for ages and it makes me sad to have to change the status back again but...
It seems like this patch is the cause of this bug where I cannot use ckeditor styles to apply classes to images inserted using the media module: #2458939: CKEditor styles are not working with media
If it is confirmed a bug in this patch I can close that issue as cannot reproduce and it can be fixed here.
Comment #174
izmeez CreditAttribution: izmeez commentedThe patch in #166 will need to be re-rolled as result of #2455391: Call to undefined function media_browser_js() latest Media-7.x-2.x-dev arising from #2454933: Remove JS helper functions
Comment #175
izmeez CreditAttribution: izmeez commentedThe change is around line 630 of includes/ckeditor.lib.inc
The line needs to be changed to
if (array_key_exists('media', $settings['loadPlugins']) && module_exists('media') && module_hook('media', 'filter_info')) {
After #2455391: Call to undefined function media_browser_js() latest Media-7.x-2.x-dev is committed.
Comment #176
rooby CreditAttribution: rooby commented@izmeez: It will only have to be rerolled if that patch is committed.
It seems to me that if this patch is committed that patch is no longer required, in whihc case this patch doesn't need a reroll.
Comment #177
aDarkling CreditAttribution: aDarkling commented@rooby, addressed your problem in https://www.drupal.org/node/2458939
As I mentioned there, there are a few other things that need to be addressed. However, there's no point in working on them while the basic functionality of Ckeditor->media_wysiwyg remains broken because this patch isn't committed.
Comment #178
rooby CreditAttribution: rooby commentedI agree, setting status back and keeping that issue separate to befixed once this goes in.
Comment #179
jcisio CreditAttribution: jcisio commentedWell... I check the patch a few times and do not find anything. It has been already tested a lot and recent feedback has been solid, so I decide to commit it, with some minor wording and coding standard fixes.
Thanks all for this yearlong issue.
Comment #181
rooby CreditAttribution: rooby commentedYay!
Thanks to everyone that worked on this, now we're a step closer to an out of the box inline media solution :)
Comment #182
marcusx CreditAttribution: marcusx commentedWooooha! Awesome that this finally got committed!
Comment #183
saltednutAmazing! I was so busy working yesterday that I missed this going in... Great work everyone.