Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
I'd like to be able to export the wysywig configurations with ctools exportables, so they can be kept in svn and moved between sites easily. Seems like this might be on the roadmap for 3.x, but couldn't find an issue, so opening one.
Comments
Comment #1
rickvug CreditAttribution: rickvug commentedYou took the words right out of my mouth. Creating a solid WYSIWYG configuration can often rely on a lot of small settings. Having exportables for those settings would be a god send.
Comment #2
TwoDGood point. Bumping this to 7.x-3.x-dev as that's where the new major features will be added first. Note that this is not a promise that it will be added, but we'll look at it.
Comment #3
xamanu CreditAttribution: xamanu commented+1 this would be great, in order to build features, like catch said.
Comment #4
dagmarYes this is an excellent feature. But, if settings of wysiwyg are related to an input format, export settings will mean export the input format too?
Comment #5
rickvug CreditAttribution: rickvug commented@dagmar You bring up a good point. Especially since input filters are set numerically, making them problematic to import and export reliably. I'm hoping that this is not the case in D7 seeing as how much of the filter module API has changed. The Exportables project may be a good place to tack on an exportable API for this. If not, WYSIWYG could always implement its own work around. In the end, I would hope that this complication doesn't side track any incremental improvements. WYSIWYG configuration exportables would still be helpful for install profiles and other use cases without input format exportables.
Comment #6
catchI'm using features on a large site upgrade to avoid having to write and rewrite custom hook_update_N() for as many things as possible, so I took an hour or so to write very, very basic features integration for wysiwyg-6.x-2.0. This doesn't attempt to resolve any of the bigger issues around exporting input formats + wysiwyg, just allows you to put the contents of the wysywig table into code and version control / revert / override it.
So I'm deliberately not even marking this needs work, but in case someone else is hoping to do the same thing, maybe it'll be useful. It's also implemented as a self-contained module rather than a patch, since that's how I'm handling these temporary upgrade feature implementations.
Comment #7
nedjoTagging.
Comment #8
mrfelton CreditAttribution: mrfelton commented+1
Comment #9
rgristroph CreditAttribution: rgristroph commentedI somehow missed this discussion when I set out to do my own WYSIWYG / features integration.
After making the settings exportable, I also made them importable, and then I made the WYSIWYG check for settings using a default_hook so that the module created by Features could provide the settings. When you export WYSIWYG settings to a feature, and then install that feature, the settings are in code and not in the database. If you then edit the WYSIWYG profile settings and save them, then those go to the database and override the ones in the feature.
I have attached a tgz of my modified 6.x-2.0 WYSIWYG. Should I also provide a patch against one of the CVS branches ? If so, which one ? DRUPAL-6--3 ?
Comment #10
rickvug CreditAttribution: rickvug commented@rgristroph Thanks for sharing! I created a patch out of your modifications to make them easier to review.
Comment #11
rickvug CreditAttribution: rickvug commentedFirst patch did not include the new file wysiwyg.features.inc (although it is blank anyhow).
Comment #12
dagmarWell I have reviewed and modified a bit this patch.
Big changes:
Exports provides as unserialized arrays (this is better to diff changes)
Same for feature support.
Small changes:
A lot of coding styles fixes.
Changed title in export page.
Mantainers: Related to #2: I'm providing this patch against DRUPAL-6--2 and DRUPAL-6--3, I have only tested this patch using DRUPAL-6--2 and I'm sure that is the branch were we are going to get more testers.
Sadly I haven't much experience developing for Drupal-7 (in fact we have to wait one week more for the first alpha for D7) so If you agree, I suggest change the version for this issue to 6.x-2.x-dev (or 6.x-3-x-dev if you enable it) to get test from more people.
Comment #13
Todd Nienkerk CreditAttribution: Todd Nienkerk commentedAwesome! Subscribing.
Comment #14
Todd Nienkerk CreditAttribution: Todd Nienkerk commentedI'm not having luck with the DRUPAL-6--2 patch. The editor settings aren't showing up in either the default "Filtered HTML" format or a custom format I defined on the source site. Perhaps the import is breaking due to the lack of the custom format, which wasn't created prior to enabling the feature?
It's also worth noting that the status of the feature is "Default," even though the settings are clearly wrong.
Comment #15
Todd Nienkerk CreditAttribution: Todd Nienkerk commentedI just tried a simple export/import of the "Filtered HTML" format's WYSIWYG setting and received an error. I suspect this is why the feature isn't working.
Here's the export:
The error:
Comment #16
Todd Nienkerk CreditAttribution: Todd Nienkerk commentedI tried the same export/import on a cleaner install. It worked this time, but I still got an error:
Comment #17
dagmar@Todd Nienkerk: Ok I will review the patch again, thanks for your test.
In a conversation via IRC whit sun, he told me that patches for 6.x version are still accepted. So I'm changing the version for this issue.
Comment #18
jmiccolis CreditAttribution: jmiccolis commentedHi Folks, I believe you're going to have some trouble with this approach. The issue stems from how WYSIWYG relates it's configuration back to the filter formats using numeric IDs and then uses these IDs itself as the primary identifier of the configuration. In order to get exportables that are useful between sites you're going to need to get away from using a numeric ID as the identifier.
I can see two possible approaches; 1) somehow use the human readable name of the format (as stored in the filter_formats table) as the id in the export. 2) store string IDs in the wysiwyg table and then some how map those back to formats.
I'm not sure which is going to work out better, but I think these have the potential to get better exports for this config.
Comment #19
TwoDjmiccolis is right, numerical ids cause problems in these cases, though very convenient in most cases. I see this is also discussed elsewhere, like #643566: Text formats and #640302: Provide exportable for input formats.
I think one way to solve this problem of relating say Wysiwyg profiles to Input formats would be to add an extra step after importing the profiles. In this step they are all listed and can be mapped to another list of the current input formats (by the user). Possibly with the ability to keep the profiles in this half-imported-state until new Input formats have been created, if there weren't enough to assign all the profiles in one go. There is of course no guarantee that a profile being assigned to a format will actually have matching settings, but that is an issue in the current state as well, even if the format ids all exist. So when exporting an editor profile, the format id is left blank. Then on import, a profile does not go straight into the database (after validation) but is instead stored in a temporary place/cache somewhere where it can have those format ids set before being put into the db (passing validation again). It's just a thought, but I think it might be possible to apply to the relationship between input formats and filters as well. However, the amount of time and user interaction it takes to import it all will of course increase a lot.
Comment #20
catchYeah I deliberately didn't set #6 to CNR or CNW because it wasn't supposed to be a real patch. Using numeric IDs works as a quick hack to save me writing a custom hook_update_N() in my specific use-case for features, but it shouldn't be used for anything more than that.
To do this properly, since core itself is using numeric ids for formats and there's not much can be done about that until D8, it might be possible to work around it with http://drupal.org/project/exportables - something like that needs to be worked out for exporting input formats first, then applied to wysiwyg/better formats etc. once it's in place so they can share the same basic metadata about what refers to what.
Comment #21
rickvug CreditAttribution: rickvug commentedFor what it is worth I tried the patch in #12 and received similar errors. On first install the feature enabled properly. After changing a setting, it showed as overridden and would not revert. See the attached screenshots for the errors and diff output. I then tried a clean install and received the same error (once). Beyond the error message and overridden status the patch seems to work as expected.
Comment #22
that0n3guy CreditAttribution: that0n3guy commentedsub...
Comment #23
drifter CreditAttribution: drifter commentedsubscribanatoring...
Comment #24
HnLn CreditAttribution: HnLn commentedsubscribe
Comment #25
drifter CreditAttribution: drifter commentedRerolled the patch in #12 for Wysiwyg 2.1. Also fixed the error in #16 (_validate() didn't have the correct arguments). Added an option to (optionally) select a target input format on import, so you can export from one format and import into another.
Apply the patch without -p0.
Comment #26
pounard@drifter #25
Works for me, I manually fixed your patch in order to have the right file path (rest of the patch is exactly the same).
A clean update function is missing for already existing sites.
Comment #27
drifter CreditAttribution: drifter commentedThere is no actual database schema change, so no update hook is necessary. The changes in .install/hook_schema are for CTools exportables support.
Thanks for the updated patch, couldn't figure out the correct git diff switches :)
Comment #28
pounardOk, finally, I got a bug :)
The first export of my features went well and got back editor options (as I remember), but then I used the feature recreate form to get back an updated version and I loosed all options. Since, I'm not able to export those options anymore.
If I found why, I'll try to patch it.
Comment #29
pounardOk found something. When you edit a "default format", the submit handler does an sql update on an empty row, so it does not save. I modified the the overview form, and edit form in order to handle defaults.
Now on the overview form, if a default profile is not present into database, the 'edit' link becomes 'override'. At edit submit time, the submit handlers checks if there is any data in database, if not, create a new record instead of modifying an empty one.
Now it works fine on my environment. Redone your patch using the diff command.
Comment #30
adrianmak CreditAttribution: adrianmak commentedsubscribe
Comment #31
bramface CreditAttribution: bramface commentedComment #32
bramface CreditAttribution: bramface commentedComment #33
bramface CreditAttribution: bramface commentedI had to manually apply hunk#1 (the extra else added to wysiwyg.admin.inc) - tried the patch (and various other patches in this thread) a few times.
-Bram
Comment #34
bramface CreditAttribution: bramface commentedWhy is it when I reply to #29 (by clicking the reply link in that message) my reply does not appear indented underneath #29? It ought to be....but that's not an issue for this case!
Comment #35
pounardDid you applied the patch on 2.1 stable release?
Comment #36
Cyberwolf CreditAttribution: Cyberwolf commentedSubscribing.
Comment #37
kim.pepperSubscribe
Comment #38
langworthy CreditAttribution: langworthy commentedI had trouble with #29 when using an exported feature definition in a *.features.wysiwyg.inc file.
I changed the following line in wysiwyg.module
to
patch attached
//edit: noted which file change was made to relative to #29
Comment #39
dagmarI have reviewed the whole patch, and I did make some changes:
I did test the import/export process using plain exports and features, they seems to be working fine. There is only a problem when users try to delete a profile from an input formats that is loading the profile from a feature. It is never deleted. (until of course, you disable the feaure) It is obvious, because the profile is loaded from the code every time if not found in the database. I don't know if we have to document this behavior, or this can be fixed in some way.
Anyway, it is working! please make your tests and provide feedback.
Comment #40
netsensei CreditAttribution: netsensei commentedLooks great... but it doesn't work for me. In features, I'll get the extra 'wysiwyg profiles' option. I can see the extra input format which I've defined. But it's naming is missing. When I click on the tickbox, nothing happens. There is no indication whether or not the format was included. The code of the feature is lacking every reference to the wysiwyg profile/input format too.Drupal 6.16 with better formats and wysiwyg module.My bet is a conflict with Better Formats.I skimmed through the code of the patch. Noticed a change in hook_schema in wysiwyg.install. Of course, I had already installed and configured wysiwyg before applying this patch. That's why it didn't work in the first place.
Now, it's working great. Even with Better Formats.
The numeric id's are definitely something which needs taking care of though.
Comment #41
nedjoLooking good. This is much needed. I haven't tested. From a code review, a few comments:
Would be slightly cleaner as:
Should be "Unable to interpret wysiwyg code."
"exits" should be "exists"
May as well use ===
Extra space after return.
$default_formats instead of $default_profiles
$default_profile or $format instead of $def_prof
$default_formats instead of $default_profiles
$default_profile or $format instead of $def_prof
While it's true these are only called from code blocks that test for ctools, they should still include the same test, in case they're called from elsewhere. Or they should be marked as private functions (function _wysiwyg_format_load).
Powered by Dreditor.
Comment #42
Agileware CreditAttribution: Agileware commentedGreat work, reviewing.
Comment #43
dagmarI have recently uploaded http://drupal.org/project/input_formats to deal with the numeric ids for input formats. Please, take a look to this module and provide some feedback :)
Maybe we can make an integration with this patch and import wysiwyg settings using machine_names instead of numeric ids.
Comment #44
dagmarOk, here is a possible approach to integrate machine names with Wysiwyg without make input formats a dependency for Wysiwyg: http://drupal.org/node/821036
ideas?
Comment #45
pounardI vote +1 for input_formats module support.
Comment #46
dagmarWell, I have worked a bit in this patch.
First to all, thanks @nedjo for the review. I have included all the tips you said.
Ok, this patch includes a lot of news features:
As you can see in the patch, wysiwyg.features.inc is bigger than in #39. Well this was necessary to allow other modules to alter this exports. Remember, I'm doing all this things to get exportables using 'machine names' instead of numeric ids.
I have committed all the wysiwyg_*_alter implementatations to modify numeric ids into Input Formats checkout from cvs or wait to alpha-2 to tests it.
Comment #47
Josh Benner CreditAttribution: Josh Benner commentedsubscribing
Comment #48
rcross CreditAttribution: rcross commentedsubscribe
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commentedSubscribing.
Comment #50
dagmarThis is a small modification of #46. Delete profiles were not possible with previous patch.
Also I forgot mention that Heine Deelstra suggest me via IRC to include a new permission to import wysiwyg profiles.
Comment #51
ZenDoodles CreditAttribution: ZenDoodles commentedYikes!
I missed this when I was implementing features/ctools support too. Changing the title so other people who search for 'features' instead of ctools might find it.
Comment #52
ZenDoodles CreditAttribution: ZenDoodles commentedIn #50, dagmar said...
"... include a new permission to import wysiwyg profiles."
I'm all for granular permissions, but this one confuses me. Help me understand under what circumstances someone would be importing a wysiwyg profile who does not have permission to administer filter formats?
Comment #53
dagmarMmm, yes, it is true, but if you see in other perspective, an administrator may want allow users to edit wysiwyg profiles without allow they to "import profiles using php code".
So, the permission is provided to allow users to edit wysiwyg's settings without allow they to execute php code and not backwards.
Comment #54
KarenS CreditAttribution: KarenS commentedThis looks very useful, I'm working on a WYSIWYG feature right now, I'll give the code a spin.
Comment #55
KarenS CreditAttribution: KarenS commentedOK, works beautifully! As far as I can see this is good enough to commit.
Just a couple of notes for anyone trying it out:
1) If you apply the patch to an existing site, you have to clear caches before the export will work, I think because schema doesn't get updated to find the export information.
2) When you set up a new site and enable this as a feature, you have to clear the caches before you will see the wysiwyg editor (filter caching or something).
I applied the patch, exported an existing configuration into a feature, set up a clean install and enabled the feature and it worked beautifully.
Comment #56
KarenS CreditAttribution: KarenS commentedI should note I was using this with http://drupal.org/project/input_formats, as noted above, to be able to export the input formats so wysiwyg could apply the editor to the format.
Comment #57
q0rban CreditAttribution: q0rban commentedSubscribe
Edit: that didn't work the way I wanted it to. :)
Comment #58
Jackinloadup CreditAttribution: Jackinloadup commentedsubscribe
Comment #59
nedjoThis is looking generally okay and the introduction of the Input formats module is a step forward, but the patch needs some more thought and work.
The main issue is the handling of numerical IDs. If it relies on numerical IDs, it seems to me, it's not a feature. If we're implementing this, we need to get it right, and that means that means machine names.
Currently, the patch reworks some code from ctools to introduce a number of hooks. The Input formats module has been patched to implement these hooks. So exports will have machine names if a site admin has thought to install input formats, if not then numerical IDs.
This sorta works, but awkwardly. First, if Input formats is an API module facilitating import and export of data related to input formats, it shouldn't be necessary to introduce a bunch of new hooks and patch the Input formats module every time another module wants to use it. To follow this model for e.g. Better formats, (see #616496: Features integration) would have to have its own versions of ctools exportables functions, introduce new hooks, and then patch Input formats.
Instead, we should start off with: Is Input formats the right approach, and if so is it stable enough to rely on? Is there further work to be done in that module before relying on it?
If input formats is the right approach, it should be introduced as a requirement for wysiwyg import/export functionality alongside ctools bulk_export. Since we'd be testing in several places for two separate modules, best to pull this test into a helper function.
Then we call methods in Input formats to load the machine names and to switch out machine names for numerical IDs.
In general, I'd like to see how another module has introduced optional ctools exportables support for comparison. Can someone suggest a module to compare?
Other smaller issues:
For consistency with existing usage, wysiwyg should be capitalized in several places, Wysiwyg.
It's not really ctools but bulk_export that we need to test for and give a message about. Installing and enabling ctools isn't enough.
Spacing issue.
I'm thinking we should revert all of this, but if it stays, there are several spacing and grammatical issues. Should read:
These hooks are not ... But since wysiwyg uses ... we are enabling other modules to provide exportables with machine names.
paramaeter should be parameter
This and several other comments need end of line punctuation (period).
$export instead of $xport.
Extra spaces at end of line.
Powered by Dreditor.
Comment #60
shiva7663 CreditAttribution: shiva7663 commentedsubscribing
Comment #61
adrianmak CreditAttribution: adrianmak commenteddoes the latest dev build patched with ctools exportables and feature support ?
Comment #62
AdrianB CreditAttribution: AdrianB commentedSubscribing.
Comment #63
steveparks CreditAttribution: steveparks commentedSubscribing.
Comment #64
dagmarI'm going to try to explain the problems I got while I was coding this patch and input formats...
The big problem we have are not numerical ids... the problem is, that filter module is part of the core, and we cannot modify it.
Other modules, like Quicktabs (#572880) can be modified to use machine_names instead of numerical ids. But, with filters we cannot do that.
There is another problem with this. CTools Exportables are expecting a machine name as key of the base table. And, due wysiwyg uses numerical ids from filters, CTools will use this numerical ids in their exports/import functions.
Due to the filters architecture cannot be modified, not until Drupal 8, I just created input_formats module to don't have to wait two years more to be able to load wysiwyg and input formats from code.
Input formats is just an API to synchronize numerical ids with machine names, plus a set of tools to export/import settings for input formats.
Is it stable? Well, it's wroking... of course probably with the time there will be things to fix, but right now is usable and I have fixed all issues I founded during my tests.
Also, why so many hooks? Again, since numerical ids are something normal in input formats, many times objects are loaded using this ids. So I had to check every load and import case to make sure that in all the ways where a wysiwyg profile was exported/imported finally a numeric id were used.
The hooks implemented in input formats were introduced to avoid a dependency for input formats. Wysiwig is a mature module, and input formats isn't yet.
I know that it is better to use a few call to input_formats api instead to use a dozen of hooks calls, I was wondering what would think others developers about this... wysiwyg depending of a new module? A module with 2 weeks of life...? Probably the patch would be rejected.
Input formats is the first module to offer a solution for numerical ids for filters. I don't know if there is another module to manage filters with machine names.
I just didn't do things mentioned in #59 because I did know that input formats wasn't ready to be a dependency of wysiwyg. But if the community thinks that input formats can be the solution for numeric ids for filters in Drupal 6 and 7... so we can work in a stable version of input formats before continue working on this patch.
This are my thoughts right now. I agree with @nedjo, wysiwyg cannot use numeric ids in features, if anybody else have another approach for this patch, it would be nice to put they here too.
BTW, @nedjo, thanks again for your review.
This was longer than I expected, sorry for my english.
Comment #65
q0rban CreditAttribution: q0rban commented@dagmar, have you thought about abstracting your code for use with the Exportables module? Essentially making it so that arbitrary numeric keyed entities can be exportable?
Comment #66
ZenDoodles CreditAttribution: ZenDoodles commentedI am working on a patch that uses hook_filter_default_formats instead. I hope to have it ready to post in a day or so.
Comment #67
arski CreditAttribution: arski commentedsub
Comment #68
wik CreditAttribution: wik commentedsubscribing
Comment #69
dagmar@q0rban: Yes it is a good idea. However I would like to see a more stable version of Exportables, like a beta or a release candidate to rely on its API.
I asked to dmitrig01 to be the co-maintainer of exportables, it seems to be the last commit for this project was 28 weeks ago...
But yes, in general, with some patches to exportables I could modify input formats to work with it.
Comment #70
Boobaasubscribing
Comment #71
pcambraI'm having a little issue with this patch, when you have more than one wysiwyg profile in a feature, it seems only to import the first one. Anyone have experienced this issue?
Comment #72
dagmarA week later, here are some news:
Thanks to @dmigrig01 and @jazzslider I'm the new co-maintainer of exportables, and recently have created a new branch of exportables to deal with the machine names.
Also, @merlinofchaos committed a patch that allows ctools exportables to use joins in tables.
The result?
A really small patch that provide full exportable support using features, the admin UI to export patches is missing, but I will code it soon.
A good new is that input formats is not a dependency anymore, and as @q0rban suggested in #65, I will port the api of input formats to use Exportables api.
To try the patch you need the last development version of ctools, and the 2.x branch of exportables.
I'm setting this as "needs review" but indeed it need a bit of work to include the UI to import/export.
Comment #73
shopdogg CreditAttribution: shopdogg commented+1
Comment #74
pcambraJust tested dagmar patch in #72 inside a feature for wysiwyg profiles and it seems ok
Comment #75
dagmarSome things to do with this patch.
If an input format doesn't exists, wysiwyg should inform to the user that there is a wysiwyg profile that cannot be installed because the input format doesn't exists.
If exportables module is not installed, {exportables} table will not exists, and therefore, a sql query warning will be displayed, maybe this needs a patch for ctools, or maybe a implementation of the hook_exportables_api with a if (module_exists('exportables')) like the patch in #50.
The UI have to be coded. I tried the new UI provided by ctools/exportables-ui, but it is too much for this module, I would like to include the UI as a separated module, something like Wysiwyg Copy (as cck does with content copy) and there, include ctools and exportables as a dependency. Opinions?
Features module is using a hard-coded implementation of ctools_export_crud_delete in ctools_component_features_revert(), this have to be modified in both, CTools and features, to support DELETE FROM table1, table2 ... using machine names. If this is not ready, revert features will not be possible.
Changing to "needs work' until finish this things.
Comment #76
that0n3guy CreditAttribution: that0n3guy commentedI've been using the patch from #46 (haven't tested the patch from 72 yet) and using it with an install profile. After an install, I have to visit (I only have to visit, don't need to save anything) admin/build/features, admin/build/imagecache, and admin/settings/wysiwyg for my feature to work right.
After visiting admin/build/features, my input formats show up. After visiting admin/settings/wysiwyg, my wysiwyg editors load after changing them in a nodes input formats. After visiting admin/build/imagecache, my imagecaches show up w/ wysiwyg_imageupload in my wysiwgy editor.
My feature works fine if I enable it from features, but not from install profile. Just thought I'd let ya know.
Comment #77
dagmarHere is the patch with the UI working. I have created a sub module called Wysiwyg Copy to provide the Import/Export UI. (something like cck does)
Now features integration are only available if exportables and ctools are installed. This prevent warning from missing tables in the db.
The UI is very similar to views UI, terms like Overridden, Revert and Export are used in the same way.
I have created the two issues mentioned in #75, I will try to provide patches for those issues as soon as I can.
Features: #853120: Use ctools functions instead of querys for revert features
CTools: #853114: Delete objects from tables considering joins
I recommend use the last development version of features (I did my test with that version) and remember to use the 2.x branch of exportables module.
Comment #78
q0rban CreditAttribution: q0rban commentedOne thing you might look at for UI implementations is the new export UI include in the dev version of ctools. :)
Comment #79
dagmarI did it, but it is too complex for wysiwyg options. Also it doesn't allow to select the wysiwyg editor with a select box, too much things to modify.
Comment #80
dagmarWell, it is working pretty good :)
This last patch includes a hook_requirements() to inform about missing Ctools or Exportables modules in the Status Report.
Also include a way to revert wysiwyg profiles directly from features UI, but it requires a patch for features #853120: Use ctools functions instead of querys for revert features
The patch for Ctools (from comment #77) is no longer required.
Comment #81
Owen Barton CreditAttribution: Owen Barton commentedI have tested a few iterations of this patch (together with both stable and HEAD versions of both ctools & features, clean installs) and have the same problem as above - it only ever shows the first WYSIWYG configuration in features.
As far as I can tell it is not joining to the exportables table at all, and hence is not picking up the machine name for the input format, causing the array items to have no key and them clobbering each other.
@dagmar - it seems like you have this pretty well figured out, any ideas why ctools isn't joining the tables correctly?
Comment #82
dagmar@Owen Barton: Yes, I know there are some issues. However, I would like to insist on this: This patch is for wysiwyg, not for input formats. If you don't have the input format installed in your site, the wysiwyg won't be installed.
My tests for #80 were done with all the input formats created (and they works fine, with two wysiwyg, both were loaded). You can export both, the input format and the wysiwyg profile in a feature, but there are some issues. I recently discovered in input formats module that are making this things not work properly.
I'm going to modify the way how input formats works in beta-2. I discovered a way to rely on strongarm and ctools to load input formats "really" from code (at this moment, input formats are copied into the db). But I need a week to create this new code. Until that, test to this patch should be done with the correct input formats properly created.
So, if anybody can confirm #71 under the conditions of testing I described above, feel free to change this issue to 'needs work' again. And of course, @pcambra, @Owen Barton, thanks for the feedback.
Comment #83
Owen Barton CreditAttribution: Owen Barton commented@dagmar, thanks for responding. I am referring to exporting the configuration to a feature (not installing the configuration using an already installed feature), if that helps. The input formats are obviously already present in the case of exporting - do you mean that the input formats need to be added to the feature (I am using your input formats module also) when exporting before the wysiwyg configuration can be added to the feature?
Loading input formats directly from code would be awesome, if that is possible - if that works it seems like it would be quite straightforward to do the same for wysiwyg also.
Comment #84
dagmar@Owen Barton: now I can replicate this problem. I'm not sure why it was working fine before...
Take a look to this patch. I think this is the raise of the problem: #862320: Skip check if 'export' exists on a schema when are used in joins
Comment #85
dagmarHere is a new patch that works with the recently created release of Exportables 6.x-2.0-beta1.
Also includes the revert capabilities using CTools 1.7 api that were not included in Features 6.x-1.0-beta10.
Comment #86
dagmar@Owen Barton: Input formats 6.x-1.0-beta 2 rely on Ctools 1.7, Strongarm and Exportables 2.x. Now input formats are loaded from code. http://drupal.org/node/869978
Comment #87
pounardCould the module just have an "export/import" function independant from CTools? This is just like using a nuclear war head to kill a tiny fly. Strongarm has really specific use cases, and is a great module, but here for defaults handling and simple import, this is just an enourmous overhead that does not proove itself to be usefull. Why does people has to use such machinery, I know that Drupal lacks the "exportable" concept (really lacks it a lot), but using 3 different modules to achieve this goal, this is just insane. Exporting a wysiwyg module profile is like 10 lines of code, no need for those modules don't you think? And shouldn't the wysiwyg module remains independant from such modules? The features module support, even if -for some people at least- is really needed, at best, would need CTools exportable, or, if you have a bit of courage, maybe 50 lines of custom code to be done.
Using CTools AND Exportables only for default profiles loading/support is crazy, and useless.
Comment #88
rickvug CreditAttribution: rickvug commented@pounard Have to disagree here. CTools is nearing ubiquity now. More modules using its exportables API means less custom code needs to be written and the simpler this all becomes. I agree that having exportable input formats and WYSIWYG settings shouldn't take a group of modules with dependencies. However this is the option we are left with when working around core's limitation while keeping things modular.
Comment #89
mrfelton CreditAttribution: mrfelton commented@pounard: Why is it an overhead to use ctools? It is not like you are exporting these things every day. What about, to mitigate against overhead - having any supporting code that has to live in this module, to live in a submodule wysiwyg_export, and let that submodule have the dependency on ctools. That way, you can just enable wysiwyg_export and ctools when you need to export. Disable them again when you are done. No overhead. Less overhead infact, since you have then removed all the export specific code out of the main wysiwyg module, which is the only one that needs to be enabled all the time.
ctools really is the way to go IMO. Individual modules should not be reinventing this stuff.
Comment #90
pounardIf you just look at the "module provided defaults" feature, fetching the defaults is something like calling a simple hook. Using CTools goes within a call stack trace with an incredible depth, forces to write all sorts of hooks, doing (really a lot) of includes, which seems a bit too much just to call a hook and enventually do (static) caching.
Remember that this particular feature (defaults) can potentially be called each time a wysiwyg editor appears.
Plus, if I look at the latest patch, there is as many lines of codes just to use CTools that it would need to implement a custom and simple hook.
EDIT: Typo (twice)
Notice: This is only IMHO, and if you choose to use CTools and Exportable I will totally respect that.
Comment #91
dagmar@pounard, thanks for your comments. I just would like to say that I have been working a lot of days around this issue, and I have examined a lot of possibilities, and this last patches are the best approach I have found. A balance between performance, don't reinvent the wheel and functionality.
As you can see in wysiwyg_profile_load_all(), all the 'hard work' is performed only if cache is not available. As I said in #46, the cache system is implemented to improve the performance of all the system. This means that wysiwyg profiles cache will be re-loaded only when you visit admin/settings/wysiwyg, and in admin/build/modules, and not every time a wysiwyg is requested.
For other side, it is true that CTools is slower than use our own code. But, as @mfelton said in #91, you will not export and import wysiwyg as a regular task, so few seconds are not so important.
Related to Exportables, I would like to said, there are not other valid options. Filter module doesn't provide machine names, and wysiwyg profiles are deeply relatd to input formats. Even more, exportables don't overhead a site, it just work in special cases, like administrative screens to keep sync the input formats with their machine names. And strongarm is no needed for this patch, strongarm is needed for input formats, other module.
I know that this stuff can be improved in terms of performance, and, in my opinion, is here were we should put our efforts, make this things works as faster as possible.
Anyway it is good to see this discussion, because we can see where are the critical points of action.
Comment #92
pounard@dagmar Where I really agree with you, is the problem of numeric input formats. There is no easy way here. But may be using the human name, or a hash of the human name, as key for import/export could be a solution, Drupal core default provided formats always have the same name? (I known, this is not really a solution, but at least, it would avoid adding non core related meta-information for those).
EDIT: In fact, the whole compexity of export comes from that fake need of machine name, which is kind of tricky there. I don't think putting machine name everywhere is the solution.
Comment #93
jide CreditAttribution: jide commentedsubscribing.
Comment #94
pounardA bit off topic note, there is a similar issue there, with somehow similar questions and problems: #558378: Make workflows exportable with Features (D6)
There is good ideas here and there, so I think you should give it a read.
Comment #95
karschsp CreditAttribution: karschsp commentedsubscribe
Comment #96
lucascaro CreditAttribution: lucascaro commentedsubscribing
Comment #97
lucascaro CreditAttribution: lucascaro commentedsubscribing
Comment #98
lucascaro CreditAttribution: lucascaro commentedWorked great for me with 6.x-1.10 (I've only changed the button settings for the two default input formats).
Thanks!
Comment #99
that0n3guy CreditAttribution: that0n3guy commentedUsing the patch at #85 gives me these errors: http://drupal.org/node/890412 untill exportables is enabled.
Comment #100
dagmar@that0n3guy exportables 2.0-beta1 is required to use this patch.
Now that features is stable and properly integrated with CTools 1.7, I'm uploading a new version of this patch. With less lines of code.
Please remember clear your caches.
Comment #101
sunSorry for coming late to the show. Thanks for all the work and reviews that have happened so far. However, I've re-opened #643566: Text formats (Features, D7), as I'm pretty sure that all of this cannot properly work without an enforced machine_name for text formats. My very basic proof-of-concept patch over there should solve the underlying problem of a missing machine_name by simply adding it at the right locations and at the right time, i.e., focusing on CUD operations. (which already works cleanly based on manual testing) Of course, the Filter API situation is heavily improved in D7, as we've completely revamped it. Among many other changes, filter settings are now stored with each filter in {filter} instead of variables, which definitely solves or simplifies a large part of the puzzle (it's not that we couldn't make that identical machine_name enhancement concept work for D6, too - actually, the idea of a machine_name module crossed my mind already).
Comment #102
sunAdditionally, I've created an almost RTBC patch for Drupal 7 core that massively simplifies the implementation of machine names: #902644: Machine names are too hard to implement. Date types and menu names are not validated Please help to get that in.
Comment #103
pearcec CreditAttribution: pearcec commented+1 sub
Comment #104
tsvenson CreditAttribution: tsvenson commentedFollowing
Comment #105
attiks CreditAttribution: attiks commentedFYI: Patch in #100 is working like a charm, thanks
Comment #106
that0n3guy CreditAttribution: that0n3guy commentedI'll second that. No problems so far.
Comment #107
sunSince almost no one of you stepped up in #902644: Machine names are too hard to implement. Date types and menu names are not validated and #903730: Missing drupal_alter() for text formats and filters yet, it is becoming less likely that we will be able to export anything (until D8).
Comment #108
Grayside CreditAttribution: Grayside commentedThis issue could really use a recap. What are the recommended patches and what are their requirements? (Including core version.)
Comment #109
nedjoNone of the patches submitted so far is recommended.
Rather, sun has led an effort, with help from several others, to get the bases for input format export into core for D7. This was done in #902644: Machine names are too hard to implement. Date types and menu names are not validated and #934050: Change format into string. The third piece, #903730: Missing drupal_alter() for text formats and filters, is not yet committed.
With these pieces in place, it should be possible to add input format export to Features in #643566: Text formats for D7 at least, with possibly a D6 backport.
When that is done, it will be feasible to write a new patch for WYSIWYG.
Comment #110
pearcec CreditAttribution: pearcec commentedPatch #100 breaks with CTools 1.8 FYI. Hopefully we can circle back around with a patch. Downgraded to 1.7 in the meantime.
Comment #111
pearcec CreditAttribution: pearcec commentedYou need to input_formats and Wysiwyg_Copy to make this useful.
I know submit a patch. I am crunched.
Comment #112
bleen CreditAttribution: bleen commentedsubscribing
Comment #113
YesCT CreditAttribution: YesCT commentedsubscribing.
Comment #114
dboulet CreditAttribution: dboulet commentedAlso subscribing.
Comment #115
TravisCarden CreditAttribution: TravisCarden commentedsubscribe
Comment #116
aaron.r.carlton CreditAttribution: aaron.r.carlton commentedsubscribe
Comment #117
quartsize CreditAttribution: quartsize commentedSubscribe.
In addition, I've attached the patch for HEAD that I've been using. It loads wysiwyg profiles using CTools exclusively, allowing Features' CTools integration to work its magic. The simplicity of making CTools a dependency appeals to me, but if this is undesirable, it should be straightforward to modify this patch to make it optional.
Comment #118
nedjoThanks for the updated patch!
With ctools now a dependency of Views, we can expect that more than two-thirds of Drupal sites will have it, so it's much less of a barrier than it was.
Comment #119
sunWysiwyg has reached a distribution level at which we cannot add a dependency on CTools for the sole sake of exportables — you neither need CTools nor Views for simple brochure and blogging sites, so the dependency is not guaranteed to exist; especially in D7. To clarify in advance, the commented out dependency line in the .info file dates back to an old idea of leveraging CTools' content type plugin framework for editor plugins; while this might still happen in the future, it's unlikely that it is going to happen for 2.x. Therefore, support for exportables needs to be optional.
That said, what's the difference to Entity module's support for exportables? Technically,
wysiwyg_profile
should be registered via hook_entity_info() anyway, so without knowing details, my current assumption is that by merely switching the entity controller class dynamically (based on whether Entity module is enabled or not), Entity module's controller class would automatically take over handling of exportables...? In other words: Let's make sure to use the latest and greatest implementation strategy here, not the (possibly dated?) D6 approaches.Comment #120
Owen Barton CreditAttribution: Owen Barton commentedCouldn't ctools be made an optional dependency pretty easily - wrapping in module_enabled/function_exists? I think requiring ctools to use exportables is very reasonable - I can't imagine anyone wanting to use exportables, but not needing ctools already installed.
Comment #121
nedjoIndeed in D7 Entity is an alternative to Ctools for providing exportables support and should be considered alongside Ctools.
Documentation on using Entity API for exportables is here: http://drupal.org/node/1021526. Easy!
Compared to Ctools, though, it looks a bit harder to make Entity API an optional dependency, since there's the need to add custom fields in hook_schema(). If we wrap those fields in a module_exists() test, how do we trigger creation of the needed fields if Entity API is subsequently enabled?
Also, can we get exportable support without triggering other, possibly not desired, Entity API handlers?
Comment #122
ZenDoodles CreditAttribution: ZenDoodles commentedI can definitely see the issue with adding a ctools dependency to an established module as sun pointed out in #119. It was in large part the reason I did not post my patch in June. It has been some time since I looked at the code, but would it not be possible to have a separate wysiwyg_exportables module which requires ctools?
If I'm thinking and remembering correctly, it would probably require a bit of glue in wysiwyg proper to work though, so perhaps it would be best as an optional sub-module? That way the sub-module could have the dependency, but if you weren't interested in exportables there would be no change.
Edit: Also, to change to Entity API would just make it a different dependency would it not?
Comment #123
quartsize CreditAttribution: quartsize commented@nedjo: Worse yet, what happens if Entity API is subsequently disabled?
The use of extra database columns for exportables is a relatively recent addition to Entity API; see #1008810: Store all exportables in the db.
Comment #124
sunThe actual required columns seem to be defined in entity_exportable_schema_fields(), and default to
status
andmodule
. We can add those columns permanently to Wysiwyg's schema.More investigation is required for the
status
column, as it seems like the values are totally not related to common "status" column values; they're describing the "origin" of the entity (code, custom, etc), but not its status (enabled, disabled, etc).EDIT: I.e., we likely want to use a custom column name of "origin" (or similar) instead of "status".
In case Entity API is not available, we merely need to make sure that configurations, which normally cannot be edited ("locked") when Entity API is enabled, can't be edited either when it is disabled. Aside from that, I don't see any problems.
Comment #125
Andoni G CreditAttribution: Andoni G commentedHi,
Someone knows if wysiwyg-624018-ctools-export-input-formats-2.patch runs well with Wysiwyg 6.x-2.2? I'm trying to do this but do not show me the Wysiwyg profile option when I create a new feature.
Thanks.
Comment #126
Andoni G CreditAttribution: Andoni G commentedI forgot clear cache, now appears. ;)
Comment #127
sunNo.
Comment #128
nedjoOnce #1034476: Load Wysiwyg profiles as entities is complete we should be able to add exportables via Entity API.
Comment #129
nedjoComment #130
Grayside CreditAttribution: Grayside commentedEntities API-based exportables is not something that can be backported to D6. Should D6 exportability really wait on that?
Comment #131
sun#1034476: Load Wysiwyg profiles as entities has been committed. Let's move on here. :)
Comment #132
glass.dimly CreditAttribution: glass.dimly commentedMany sites are still using Drupal 6, and I think the D6 versions of this patch are valuable.
More later.
Comment #133
nedjoHere are some first steps towards a patch.
* In hook_schema() and hook_update_x(), add the two fields used by Entity API for exportables. I've retained the field name "status". "origin" seemed inappropriate, since a particular setting might originate in code but have an override. "state" is probably better, but in any case this should be fixed if anywhere in entity.
* Add an 'exportable' key to hook_entity_info(). Also designate a 'label' entity key (since this is needed for Features to label the component being exported).
* Pull the controller class into an include that can be loaded conditionally.
With this in place the "WYSIWYG feature" type shows up under Features "Add components" and existing configurations can be selected and exported to a feature, but on my testing so far the exported code isn't loaded.
As well as fixing this issue, need to test for status (state) and offer a button label (revert) as appropriate. Part of that's in the patch in #117.
More generally, this is already a long issue. Let's reserve it for actual work on the D7 patch. When the fix is in for D7 anyone interested can return to consider if/how a clean D6 solution can be accomplished.
Comment #134
nedjoTraced the import failure to an issue in Entity API, #1047032: Imports fail for entity types without custom entity class--is_new flag not set.
Comment #135
nedjoWe'll probably need to change the way configuration is saved. Currently, when the main wysiwyg form is submitted, a record is created for each text format, even if the values are entirely empty. So e.g. if the form is submitted and later a module is enabled that provides a default WYSIWYG configuration for one of the existing text formats, the new default will already be overridden. To fix this we'll probably need to ensure we create records only if they are non-empty and different from the default. I included similar code in a draft patch for Captcha, #825088: Exportables support for CAPTCHA points.
Comment #136
sun#1047068: Redirect to profile edit form after assigning editor (and do not save upfront)
The manual inclusion of the include files needs to go. Instead, we need to rename the second controller to WysiwygProfileEntityController and dynamically assign either that or the other one as 'controller class' in hook_entity_info().
Exceeds 80 chars, but let's remove the comment entirely.
Hm. One bit item on the current roadmap is to allow to disable wysiwyg profiles (in correlation with allowing to attach more than one profile to a text format)... Will this status property allow us to mark a profile as disabled (regardless of Entity module or exported code)?
Let's always and unconditionally use the raw/internal value.
Missing default, always required for 'not null' => FALSE
Powered by Dreditor.
Comment #137
fagoIf you have marked 'settings' as serialized in your schema, the entity API will take care of that for you.
Comment #138
nedjo@sun: fixes done. "Will this status property allow us to mark a profile as disabled (regardless of Entity module or exported code)?" By default, no. Possibly if we extend the bit flag defines? And yes, #1047068: Redirect to profile edit form after assigning editor (and do not save upfront) sounds exactly right.
@fago: thx, that cleans it up nicely.
#1047032: Imports fail for entity types without custom entity class--is_new flag not set is now committed (thx fago) so the dev version of Entity API is needed for testing. Opened another related issue on Entity API, #1047704: Add Entity API as a dependency when generating features. Until that's fixed it's necessary to manually add Entity API as a dependency when adding wysiwyg_profile items to a feature.
Comment #139
IsaMic CreditAttribution: IsaMic commentedHi,
I adapted the patch version http://drupal.org/files/issues/wysiwyg-624018-with-ui-4.patch (comment #100) to be compatible to the new Wysiwyg version 6.x-2.3.
Comment #140
sun@nedjo: Thanks for re-rolling!
@fago: Do you have any recommendation for us regarding (ab)use of the 'status' column/property? The goal is to allow wysiwyg profiles to be disabled (like many other exportables). It looks like Entity API's 'status' property bit flags only denote the "origin" and behavior of an entity (custom|database|in-code|default), but not really the common sense of an entity 'status' (i.e., "publishing status").
Hence, we'd either use a different column name than 'status' (perhaps something along 'export'), or we'd add and use a custom bit flag (e.g., ENTITY_ENABLED = 0x1).
Comment #141
fago>Hence, we'd either use a different column name than 'status' (perhaps something along 'export'), or we'd add and use a custom bit flag (e.g., ENTITY_ENABLED = 0x1).
I thought about doing that too already, however I don't see any benefit in adding it to the bit flag except for making querying for it *much* harder. That said, I think it would even break existing queries in the entity API required for managing entities in code upon module (un)installation (mark entities overridden, etc.). So best, just add a separate column. In Rules, I've used $rule->active = TRUE|FALSE.
Comment #142
nedjo@fago
Thx for clarification. Further question. I added the 'module' key to wysiwyg_entity_info().
Is it needed for exportable support or can we drop it?
Also, note to self for next reroll:
Extraneous comment cut/pasted from elsewhere.
Powered by Dreditor.
Comment #143
fago>Is it needed for exportable support or can we drop it?
No, that's needed, e.g. for the features integration to properly populate the dependency.
Comment #144
sunok, was equally concerned about query performance. Let's use a separate column then. However, we want to use 'status' for the actual 'status' (enabled/disabled) like all other core entities, so this Entity API "export status" needs to use a different and custom name. AFAICS, Entity API supports a custom name (perhaps untested? ;), so this leaves us with the question of a proper name. ;)
Options: export, export_status, export_state, state, entity_status, ...
In case there won't be better suggestions, I'd suggest to use either "export" or "entity_status". Actually I hope someone steps up with a better suggestion ;)
Comment #145
fago>However, we want to use 'status' for the actual 'status' (enabled/disabled) like all other core entities, so this Entity API "export status" needs to use a different and custom name.
Hm, I'd not say node.status or user.status is really the same, as they are not about enabling/disabling configuration.
Anyway for renaming, just set 'entity keys - status' to your key and adjust the schema accordingly. "export_status" would make sense to me.
Comment #146
ergonlogicFYI, I just posted #1060846: Exportables and Features support for 6.x, since this issue is focused on a 7.x solution that can't be backported to 6.x.
Comment #147
nedjoIdeally we'd maintain consistency with other exportable component types.
Ctools exportables, and hence much of what's currently in features, uses a "disabled" property. E.g. for Strongarm variables:
Comment #148
nedjoOpened a related issue on Ctools and Entity API: #1072838: Harmonize exportables support and UI between Ctools and Entity API.
Comment #149
Bußmeyer CreditAttribution: Bußmeyer commentedsubscribing
Comment #150
DamienMcKennaRe-rolled #138 against the latest git, the only difference being a -1 offset.
Comment #151
DamienMcKennaJust to mention it, patch #138 appears to work rather well, perhaps someone could post a summary of the remaining issues so we can try to get it finished & committed ASAP? Thanks all.
Comment #152
AdamGerthel CreditAttribution: AdamGerthel commentedsubscribing
Comment #153
GregoryHeller CreditAttribution: GregoryHeller commentedboth the patch in #150 and #148 failed to apply properly for me. I'm no pro at applying patches though, so perhaps I did something wrong. 2 hunks applied, 1 failed. here is the contents of wysiwyg.module.rej for the patch in #150
Comment #154
nedjoHi Gregory,
Make sure you're applying the patch to the dev version--it requires a patch that was applied but has not yet reached stable.
If you want to try out the results without patching, you could try Open Outreach, which includes a feature (debut_wysiwyg) based on this patch.
Comment #155
rickvug CreditAttribution: rickvug commented@GregoryHeller Yesterday I cleanly applied patch #150 against 7.x-2.x-dev. Something to note is that new patches from git require -p1 rather than -p0 like CVS patches. If you are using Drush Make you need to apply the patch at http://drupal.org/node/745224#comment-4302660 to support both formats. Hope this helps!
Aside from this the patch at #150 is working great so far. If I run into any issues I'll be sure to post. Very happy to see this long standing problem essentially solved!
Comment #156
rickvug CreditAttribution: rickvug commentedI will also say that the patch looks good. Code style looks good, the schema change is there on both install and via an update hook. I'm not familiar with Entity API but the changes are simple enough and it certainly work. Anyone else for a review and to mark RTBC?
Comment #157
flosuter CreditAttribution: flosuter commentedsubscribing
Comment #158
jason.fisher CreditAttribution: jason.fisher commentedsubscribing
Comment #159
thomjjames CreditAttribution: thomjjames commentedsubscribing
Comment #160
TravisCarden CreditAttribution: TravisCarden commentedUpdating title...
Comment #161
dboulet CreditAttribution: dboulet commentedFixing typo...
Comment #162
ar-jan CreditAttribution: ar-jan commentedPatch in #150 applies cleanly to latest dev, and everything seems to work with debut_wysiwyg.
Comment #163
aschiwi CreditAttribution: aschiwi commentedI second #162, patch in #150 applies well and works.
Comment #164
rv0 CreditAttribution: rv0 commented#150 working fine for me, nice work guys!
Comment #165
Jackinloadup CreditAttribution: Jackinloadup commentedLooks like RTBC. Change if incorrect.
Comment #166
barraponto CreditAttribution: barraponto commentedtrying to set a wysiwyg profile returned an error, which seems to come from Entity API, i think this is related
Fatal error: Class name must be a valid object or a string in MYSITE/includes/common.inc on line 7394
I was trying to set latest CKEditor for Filtered HTML
Comment #167
quartsize CreditAttribution: quartsize commentedThis patch is not finished. Try, for example:
I will edit later with a better explanation [EDIT: posted #173 instead], and perhaps later a patch, but this issue should not be RTBC.
Comment #168
nedjo@quartsize: Thanks for the testing and feedback, very valuable.
It's not immediately clear whether the bug you're seeing indicates a problem in the patch or instead in Entity API's features support. On the face of it it sounds more like the latter.
Comment #169
DamienMcKenna@quartsize: *cough* or a bug in Features *cough* =)
Comment #170
cossovich CreditAttribution: cossovich commentedI think I'm doing it wrong... I've applied the patch from #150 on the latest HEAD from the Features git repo (it applies nicely and I see the changes) but when I install the module in a clean version of D7 I'm not seeing the option for "Wysiwyg profile" when I create a new feature. Any ideas?
Comment #171
logaritmisk CreditAttribution: logaritmisk commentedsubscribing
Comment #172
moritzz CreditAttribution: moritzz commentedBecause we did not want to patch Wysiwyg and depend on Entity API we tried to implement the approach ZenDoodles proposed in #122: We implemented a small module with the sole purpose of exposing Wysiwyg profiles to features. You can find the module attached to this post. (Same approach as fe_block of features_extra, based on the code of features.filter.inc from features.)
Note however: Unfortunately a revert of a feature (i.e. “drush fr [wyswig_feature_name]”) does not get applied as long as you do not clear all caches (i.e. “drush cc all”). We are not sure about that, but as far as we can tell this seams like an issue of the features module.
Comment #173
quartsize CreditAttribution: quartsize commented@cossovich: have you enabled entity API module?
@nedjo, DamienMcKenna: (this is not a rhetorical question) is it a third-party's responsibility to clear wysiwyg's extra cache? Whoever's responsibility it is, I believe it must be taken care of before wysiwyg entity exportables can be called done.
Here's an updated patch; it uses entity_delete when entity API module is enabled. So entity API module's defaults get rebuilt, and the (feature-provided) profile does not go away. Revert also works, although wysiwyg's cache needs to be cleared in order to see the results.
But as I suggested above, that's not my only issue with this patch:
Comment #174
cweagansOkay, so what's left to do before this can be closed?
Comment #175
benoit.pointet CreditAttribution: benoit.pointet commentedsubscribe
Comment #176
quartsize CreditAttribution: quartsize commentedUpdated patch: resets the columns Entity API module manages to their defaults (ENTITY_CUSTOM, no module) if Entity API module is disabled, ensuring that they remain valid if the rows are changed, or other modules enabled/disabled, before Entity API module is reenabled.
@cweagans: To do, in my opinion:
wysiwyg
table without going through Entity API module (in particular, useentity_save()
) when Entity API module is enabled, so that its expectations are met and it can manage its columns properly.ENTITY_FIXED
in whatever the appropriate way is.Though before doing this independently, it may be worth seeing if there's standard code in Entity API module for exposing a UI. In addition, the UI work should also address nedjo's concern in #135 (overriding all profiles when the main form is submitted).
status
column).wysiwyg_profile_load_all
, and are cleared bywysiwyg_profile_cache_clear
.EntityAPIController
does not (and cannot be expected to) know to callwysiwyg_profile_cache_clear
when updating thewysiwyg
table. This is okay if thewysiwyg
table is only ever updated via Wysiwyg module. But by implementing Entity API module, we imply thatentity_save('wysiwyg_profile', ...)
should work just as well.EntityAPIController
does callresetCache()
, which could be overridden to clear the static and persistent caches, but it is not properly the controller's responsibility to clear a cache implemented externally. Moreover,entity_load('wysiwyg_profile', ...)
would still ignore the cache. It makes the most sense to me to move the caching into the controller, but it looks like one would have to duplicate most ofDrupalDefaultEntityController
, which makes me a little hesitant.Comment #177
xgmorkx CreditAttribution: xgmorkx commentedThanks for your work on this @IsaMic (comment #139), will this work with CTools 1.8 too?
Comment #178
fagoad #176:
While the entity API assumes you are using for full CRUD, I don't think you'll necessarily have to use it. However, yes then you'll have to care about setting the right exportable status yourself + calling entity_defaults_rebuild() when an entity got reverted/deleted.
>Though before doing this independently, it may be worth seeing if there's standard code in Entity API module for exposing a UI.
Beside the whole re-usable admin UI it provides theme functions for the exportable entity status and one for the label that also shows the machine name.
Comment #179
alanburke CreditAttribution: alanburke commentedSubscribe
Comment #180
Anonymous (not verified) CreditAttribution: Anonymous commentedI tried the patch in #176, but I'm getting a PDO exception when I go to the WYSIWYG configuration:
PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'base.status' in 'field list': SELECT base.format AS format, base.editor AS editor, base.settings AS settings, base.status AS status, base.module AS module FROM {wysiwyg} base
WYSIWYG 7.x-2.x
Entity API 7.x-1.0-beta8
Comment #181
quartsize CreditAttribution: quartsize commented@FilmKnurd: I'm having trouble reproducing this error; did you run the database update?
Comment #182
Anonymous (not verified) CreditAttribution: Anonymous commented@quartsize: Ahhhhh, rookie error. Yeah, that's what I failed to do :-)
Comment #183
jide CreditAttribution: jide commentedSame patch as #176, but manually (sigh) edited to be applied using drush make.
Comment #184
mrfelton CreditAttribution: mrfelton commentedPatch in #176 seems to be working ok in my pretty limited testing.
Comment #185
Shadlington CreditAttribution: Shadlington commentedSubbing
Comment #186
girishmuraly CreditAttribution: girishmuraly commentedAs per the beta9 release of Entity module;
"There are important API changes for all modules that implement exportable entities with the help of the entity API module. Short, entity_load() now always returns numeric ids (instead of names as it was the case previously) and exportables have to use the now separated EntityAPIControllerExportable class."
Patch #183 has been edited to include the above change.
Comment #187
rickvug CreditAttribution: rickvug commentedTested the patch in #186. It fixes the problem with Entity API and works as good as past versions (I believe I was still using #150). I'm able to use "drush features update" to push WYSIWYG back into the feature. Reverting (tested through the UI) doesn't work properly. The feature will say that differences no longer exist but upon checking WYSIWYG settings show that nothing has actually been reverted.
Comment #188
pontus_nilssonPatch in #186 did the trick.
Comment #189
joeysantiago CreditAttribution: joeysantiago commentedSorry, i tried to apply the patch #186 but had no luck... probably i missed something previously in discussion?
i am using wysiwyg-6.x-2.3. Downloaded the patch file in my sites/all/modules/wysiwyg/ folder and launched the command
patch -p0 <wysiwyg-entity-exportables-624018-176_1.patch
this is what i got:
thanks for helping me out :)
Comment #190
paskainos CreditAttribution: paskainos commentedsubscribing
Comment #191
girishmuraly CreditAttribution: girishmuraly commented@joeysantiago are you using beta9 release of entity module? http://drupal.org/node/1190594
Comment #192
rickvug CreditAttribution: rickvug commented@joeysantiago The patch is only for Drupal 7 and will not be backported as Drupal 6 does not have Entity API or input formats with machine names. #1060846: Exportables and Features support for 6.x is the issue you want.
Comment #193
kolafson CreditAttribution: kolafson commentedsubscribing
Comment #194
benanderson CreditAttribution: benanderson commentedsubscribe
Comment #195
lpalgarvio CreditAttribution: lpalgarvio commented+1
Comment #196
drupal_was_my_past CreditAttribution: drupal_was_my_past commented+1
Comment #197
samhassell CreditAttribution: samhassell commentedsubscribe
Comment #198
likewhoa CreditAttribution: likewhoa commentedsubscribing
Comment #199
rv0 CreditAttribution: rv0 commentedabout to test patch in #186, but it didn't apply for me the way patches usually do, so I edited that (changes to file headers using the a/ b/ format)
so here's a new version:
Comment #200
rv0 CreditAttribution: rv0 commentedreporting that #199 patch is working fine for me.
What to do with the Entity API dependency? how can a user know he has to enable Entity AP to make the wysiwyg module settings exportable?
Comment #201
betoscopioPatch #199 is working for me too. Using entity-7.x-1.0-beta10 and wysiwyg-7.x-2.1. Is necessary to run update.php after patch to work.
Comment #202
pearcec CreditAttribution: pearcec commentedsub
Comment #203
PI_Ron CreditAttribution: PI_Ron commentedJust arriving in this thread.. Its massive. edit - just read above post
Comment #204
mortendk CreditAttribution: mortendk commentedsub.
Comment #205
sfyn CreditAttribution: sfyn commentedOn Drupal 7.7, using the patch from #199, I was greeted by this error message when I went to recreate an existing feature:
Comment #206
derhasi CreditAttribution: derhasi commentedpatch in #199 works fine.
@sfyn, you have to run update.php as the database table changes for that patch.
Added some lines to the README, so the weak dependency on entity is documented. Patch atttached.
Comment #207
derhasi CreditAttribution: derhasi commentedComment #208
quartsize CreditAttribution: quartsize commentedThis wording implies that Entity is a (non-weak) dependency.
How does the attached wording look?
Comment #209
quartsize CreditAttribution: quartsize commentedderhasi had changed the status; I believe the to-do list in #176 still stands.
Comment #210
das-peter CreditAttribution: das-peter commentedsubscribe
Comment #211
smk-ka CreditAttribution: smk-ka commentedSorry for being late to the party, but after we had the entity based approach in use for a couple of weeks, I've come to the conclusion that entities are NOT the right way to export WYSIWYG profiles. First, they're 100% configuration, and not something you'll want to work with e.g. inside Views (where they're currently appearing).
Another issue were stalled configurations (probably a bug in the Entity API), where I had changed WYSIWYG a profile, but those changes were not reflected in the re-exported feature.
Yet another issue was dependency management: something that Features support, Entitiy API doesn't, so you have to be careful to also select the text format ("filter format") whenever you want to export a WYSIWYG profile through entities.
So I've taken the initial code from @moritzz posted in #172 and turned it into a simple features drop-in – no need for a separate module.
Comment #212
tnightingale CreditAttribution: tnightingale commentedHere's the patch from #208 formatted for drush make's "-p0" method.
Comment #213
fagoad #211:
>First, they're 100% configuration, and not something you'll want to work with e.g. inside Views (where they're currently appearing).
A wysiwyg profile is already an entity, not? Also, I don't see why it shouldn't. Views integration for exportables might be useful in some cases and as comes for free, why not have it? Still, you can easily disable it if you prefer.
Also see the related issue #1239062: determine entity types being configuration.
>Another issue were stalled configurations (probably a bug in the Entity API), where I had changed WYSIWYG a profile, but those changes were not reflected in the re-exported feature.
I never ran into that. Feel to create an entity api issue for tracking the bug down though!
>Yet another issue was dependency management: something that Features support, Entitiy API doesn't, so you have to be careful to also select the text format ("filter format") whenever you want to export a WYSIWYG profile through entities.
Why should the entity API handle dependency management? You mean its Features integration? Currently, you'll have to care about piping through your dependencies yourself.
Comment #214
alanburke CreditAttribution: alanburke commentedI've been using the patch at #208.
it works fine for exporting a profile, but once its exported, that it.
If you to update the feature via Drush, all customisations to the profile are lost.
Comment #215
jerrac CreditAttribution: jerrac commentedSubbing.
What's the chances of this getting finished and into the module soon?
Comment #216
fabsor CreditAttribution: fabsor commentedSubscribing
Comment #217
bforchhammer CreditAttribution: bforchhammer commentedThis actually seems to work fine for me, using the patch in #212 and these module versions: WYSIWYG 7.x-2.1, Entity 7.x-1.0-beta10 and Features 7.x-1.0-beta3
I tested the following:
drush fu featurename
brought the change into feature code as expected.So, assuming that entities are still the right approach (#211,#213), is quartsize's todo in #176 still the current status for this issue?
----------
Edit: scap that, it doesn't quite work for me either... not sure what was different when I tested before. Now everything I change in the UI (i.e. have in the DB) gets wiped everytime I clear the cache or try a
drush feature-update
.Comment #218
mstrelan CreditAttribution: mstrelan commentedSubscribe
Comment #219
droplet CreditAttribution: droplet commentedSubscribing
Comment #220
Mark Vincent Verallo CreditAttribution: Mark Vincent Verallo commentedPatch in #212 does not work with WYSIWYG 7.x-2.1
Comment #221
paulbredenberg CreditAttribution: paulbredenberg commentedsubsrcibe
Comment #222
brentimus CreditAttribution: brentimus commentedsubscribing
Comment #223
that0n3guy CreditAttribution: that0n3guy commentedattached is #211 as a patch instead of zip.
Comment #224
hctomsubscribing
Comment #225
njcheng CreditAttribution: njcheng commentedSubscribing...thanks for everyone's hard work on this.
Comment #226
aschiwi CreditAttribution: aschiwi commented@njcheng Please use the new Follow button at the top of an issue to subscribe to it.
Comment #227
yareckon CreditAttribution: yareckon commentedUsing patch in #223 I am not seeing wysiwyg settings applied upon turning on the feature. No error messages in watchdog.
Comment #228
justafishI've tried the patch in #223 and it's working well for me.
Comment #229
akamaus CreditAttribution: akamaus commentedPatch #223 works for me too with wysiwyg-7.x-2.1, all settings to wysiwyg profile were applied.
Comment #230
tsvenson CreditAttribution: tsvenson commentedBesides that the .module part needs to be applied manually the patch works just fine with the current dev version.
Comment #231
sun#211 convinces through simplicity.
Added undocumented include
'file'
info to the API hook to facilitate auto-loading, adjusted to account for the edge-case of profiles mapping to no format, and committed and pushed to 7.x-2.x.Comment #232
tsvenson CreditAttribution: tsvenson commented@sun: Hm, can't seem to find what repo it got committed to. Looking at http://drupalcode.org/project/wysiwyg.git nothing has been committed to 7.x-2.x for 3 months.
Comment #233
sunAhem, forgot to push - thanks ;)
Comment #235
patcon CreditAttribution: patcon commentedHere's a drush_make patch that should apply cleanly to 7.x-2.1 for anyone who needs it
Comment #236
patcon CreditAttribution: patcon commentedAnd that's a re-roll of #223/#211, for the record (the non-entity approach)
Comment #237
boombatower CreditAttribution: boombatower commentedThis seems like a fairly important/exciting feature that might be worth another release? Personally, I will be pulling the repos at this commit just for this feature.
Comment #238
Nick Lewis CreditAttribution: Nick Lewis commentedReopening per #237.
Comment #239
sunSee #1340052: New Wysiwyg 7.x-2.2 Release?
Comment #240
whitingx CreditAttribution: whitingx commentedThis patch - http://drupal.org/node/624018#comment-5278644 - worked excellently for me - thanks very much.
Comment #241
MiSc CreditAttribution: MiSc commentedReroll of patch in comment #183 to be applied to latest dev (commit 83476bf)
Comment #242
MiSc CreditAttribution: MiSc commentedAnd here is the patch :-)
Comment #243
bforchhammer CreditAttribution: bforchhammer commented@MiSc: this issue is marked as closed and fixed, it's therefore very unlikely that someone will look at your patch. Besides, support for exportables has been a part of of wysiwyg since almost 10 months. If there's a bug or something missing in the current implementation you should open up a new issue!
Comment #244
MiSc CreditAttribution: MiSc commented@bforchhammer, interesting, when using the latest dev, exportables did not work with my setup, but with the patch it does, so maybe I have a problem somewhere else. Anyhow, this solves it for me :-). But I do not need a review of this, just somewhere to place my patch so I could use it in my make file.
Comment #245
tsvenson CreditAttribution: tsvenson commented@MiSc: Strange, I just updated wysiwyg from 2.1 to current 2.x-dev on my local test machine and got it working. It adding a new item "Wysiwyg profiles: wysiwyg" in the "Edit components" drop down menu on the admin/structure/features/create tab.
Comment #246
aschiwi CreditAttribution: aschiwi commentedfeatures export of wysiwyg profiles in dev works for me too.
Comment #247
MiSc CreditAttribution: MiSc commentedDuh. It was a problem in my make file, I thought I checked out revison 83476bf, but I also after that checked out a older version. So do not bother about my patch. Sorry.