To enable this module to be exported as features, we should implement ctools export API.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

guybedford’s picture

More than happy to review and commit patches.

Thanks for the good suggestion.

recidive’s picture

@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?

guybedford’s picture

While 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.

recidive’s picture

Status: Active » Needs review
FileSize
5.28 KB

Alright, 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.

recidive’s picture

FileSize
5.68 KB

Making 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.

recidive’s picture

FileSize
5.72 KB

Adding 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.

guybedford’s picture

Great! I will check it out.

Thanks a lot for the contribution.

guybedford’s picture

I'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.

recidive’s picture

Status: Needs review » Needs work

Features 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.

guybedford’s picture

It 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.

recidive’s picture

Status: Needs work » Needs review
FileSize
9.53 KB

@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.

guybedford’s picture

I'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.

recidive’s picture

@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.

recidive’s picture

Title: Add support for ctools export » Add support for export/import through UI and features module

Changing title to reflect the direction we are taking here.

guybedford’s picture

Great. 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.

recidive’s picture

@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.

guybedford’s picture

Status: Needs review » Reviewed & tested by the community

Glad to hear it's been tested. You should have the git and release permissions now so feel free to commit!

recidive’s picture

Status: Reviewed & tested by the community » Fixed

Cool, 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.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.