Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
To enable this module to be exported as features, we should implement ctools export API.
Comment | File | Size | Author |
---|---|---|---|
#11 | 1371202.patch | 9.53 KB | recidive |
#6 | 1371202.patch | 5.72 KB | recidive |
#5 | 1371202.patch | 5.68 KB | recidive |
#4 | 1371202.patch | 5.28 KB | recidive |
Comments
Comment #1
guybedford CreditAttribution: guybedford commentedMore than happy to review and commit patches.
Thanks for the good suggestion.
Comment #2
recidive CreditAttribution: recidive commented@guybedford: are you ok with making this module dependent on ctools? Alternatively we can a. make the import/export code conditional on the existence of ctools, or b. make the export/import feature into a sub module.
What do you think?
Comment #3
guybedford CreditAttribution: guybedford commentedWhile there's really no big issue with being dependent on ctools, it would certainly be more flexible if the detection was performed as in a, which sounds like a much more flexible solution.
If you'd be happier working on it as a sub module, then feel free to do that as well, but it would be nice to be an included feature. There's no reason you couldn't also be a co-maintainer if there are various commits.
Comment #4
recidive CreditAttribution: recidive commentedAlright, I went with a.
Patch attached, depends on #1371200: Add machine name to wysiwyg templates, so test may fail. I've tested exporting/importing templates and it's working fine.
To test, first apply patch in #1371200: Add machine name to wysiwyg templates, then apply this one.
Comment #5
recidive CreditAttribution: recidive commentedMaking export link conditional too, otherwise it would lead to a 404 page when ctools is not present.
To test, first apply patch in #1371200: Add machine name to wysiwyg templates, then apply this one.
Comment #6
recidive CreditAttribution: recidive commentedAdding export/api/api property to schema, as it is required by features module.
Again, to test, first apply patch in #1371200: Add machine name to wysiwyg templates, then apply this one.
Comment #7
guybedford CreditAttribution: guybedford commentedGreat! I will check it out.
Thanks a lot for the contribution.
Comment #8
guybedford CreditAttribution: guybedford commentedI've tested this, and everything seemed to be working well. Nice work.
I've committed it to the latest d7 version of the module, and will await further review.
Comment #9
recidive CreditAttribution: recidive commentedFeatures importing is not working, since UI and CRUD are not prepared to deal with templates from code (default hooks).
I suspect we will need to either: a. change the export/import code to not use CTools, just standard features hooks, b. patch CTools to allow "faux exportables" i.e. when objects from default hooks get imported to database, or c. make it fully dependent on CTools, make use of CTools CRUD APIs and replace the UI with CTools Export plugin so this will make the UI work for templates from code and from database.
Option "a" seems the easiest and possible best way around this.
Comment #10
guybedford CreditAttribution: guybedford commentedIt sounds like option a is definitely the best way to sort this. The whole point is to get the features support anyway - I should have checked this.
Would it be straightforward to port the ctools hooks to features hooks? Thanks for following up.
Comment #11
recidive CreditAttribution: recidive commented@guybedford it looks like you've pushed your commit of the previous patch only to 7.x-2.6 tag, it isn't on the 7.x-2.x branch.
Here's a follow up patch which get rid of ctools dependency for export/import feature and make the features integration work.
Comment #12
guybedford CreditAttribution: guybedford commentedI've only been able to read the code as I'm away from my home computer at the moment. Does this change create a dependency on the features module, or will it be an optional dependency? Will the detection of the features work like the ctools version did? This flag can then be combined with the features permission as well.
Comment #13
recidive CreditAttribution: recidive commented@guybedford: no, the export/import through the UI will work without features. But export/import through features will, of course, only work if features module is enabled.
Comment #14
recidive CreditAttribution: recidive commentedChanging title to reflect the direction we are taking here.
Comment #15
guybedford CreditAttribution: guybedford commentedGreat. I still need to test this properly. Unfortunately I'm away on holiday until the end of Feb now. Happy to make you a co-maintainer in order to get this committed sooner if you'd like. Let me know.
Comment #16
recidive CreditAttribution: recidive commented@guybedford, sorry, I ended up doing other stuff and missed your update here.
I have folks using this patch for some time now, without any problems.
If you want to give me commit access, please go ahead and I can commit this and other patches, such as the permission one.
Thanks.
Comment #17
guybedford CreditAttribution: guybedford commentedGlad to hear it's been tested. You should have the git and release permissions now so feel free to commit!
Comment #18
recidive CreditAttribution: recidive commentedCool, thanks @guybedford. I committed the patch to 7.x-2.x branch.
For some reason the latest code wasn't in the 7.x-2.x branch, but only in 7.x-2.6 tag. I fixed this too.