Mappings should be exportable to code just like views or context exports.

CommentFileSizeAuthor
#23 433388-22_exportable_mappings.patch17.7 KBaron novak
#22 433388-21_exportable_mappings.patch17.8 KBalex_b
#20 433388-20_exportable_mappings.patch17.66 KBaron novak
#19 433388-19_exportable_mappings.patch17.35 KBalex_b
#12 433388_exportable_mappings.patch15.34 KBaron novak
#5 feedapi_mapper_export_1.patch4.4 KBaron novak
#3 feedapi_mapper_export.patch4.18 KBaron novak

Comments

alex_b’s picture

alex_b commented

http://skitch.com/alexbarth/bmsut/development-seed-technological-solutio...

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Assigned: Unassigned » aron novak

will be done tomorrow

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Active » Needs review
StatusFileSize
new feedapi_mapper_export.patch4.18 KB
alex_b’s picture

alex_b commented
Status: Needs review » Needs work

Let's use a hook_feedapi_mapper_default() for declaring default mappings:

function hook_feedapi_mapper_default(&$mappings) {
   $mappings['node_type'] = array(...);
   $mappings[1234] = array(...);
}

You see that only one module will be able to declare a mapping for one node or one node type. This could create funny behavior but I think this is good for the moment.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Needs work » Needs review
StatusFileSize
new feedapi_mapper_export_1.patch4.4 KB

Now the exported stuff should be put into that hook.

Ian Ward’s picture

Ian Ward commented

I tested this, works great. One suggestion I have is to show/indicate when a map is overridden, and allow the user to "revert" back to the default setting. It would also be great to click to see a diff of the default map versus the overridden version, like the features module. What do you think about that?

alex_b’s picture

alex_b commented
Status: Needs review » Needs work

Indicate whether overridden is essential. Diff nice to have. I'd say let's get the patch in with the 'overridden' indicator and create a diff feature request on a separate follow up issue.

alex_b’s picture

alex_b commented
Version: 6.x-1.x-dev » 6.x-2.x-dev

I've thought more about this and I realize that if we have exportables we need to be able to have a UI to manage mapping sets. Say we ship a module like FeedAPI Node or FeedAPI Data with a default mapping. Now a user overrides this mapping for their specific needs. The user wants to be still able to export this mapping to code.

But with this patch he couldn't because there is already a default mapping defined in code by a contrib module.

The solution is an explicit managing of mapping sets. Imagine you can save a mapping, create a new one and then pick which mapping to use for a specific node or a specific content type by setting it to active just like you pick a theme or the default filter for your site.

alex_b’s picture

alex_b commented

We should use Ctools as it has features integration for free #532256: CTools/export integration - Including Panels export

alex_b’s picture

alex_b commented
Title: Make mappings exportable » Make mappings exportable with CTools/export
alex_b’s picture

alex_b commented
Priority: Normal » Critical

This is critical for beta 1.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 433388_exportable_mappings.patch15.34 KB

Known problems / things to do:
- no revert link for the content-type mappings
- When you visit a node and exports the mapping and the node uses the content-type's mapping, this will result in an empty object.
Unfortunately it's not possible to simply do ctools_export_object('feedapi_mapper', array_pop(ctools_export_load_object(....)) here because of the complexity of per-node and per-content-type settings.

alex_b’s picture

alex_b commented

Should we not have a nid(unsigned) and a type(varchar) column instead of a single param column?

alex_b’s picture

alex_b commented

"Unfortunately it's not possible to simply do ctools_export_object('feedapi_mapper', array_pop(ctools_export_load_object(....))"

- I think we can't solve this because of the issue that node id's can't be unique. Node mappings should not be exportable.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

#13: any benefit of having two columns?

alex_b’s picture

alex_b commented

#16: no mixing of field types, clear name for the purpose of the field ('type', 'nid')

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

#16: which one should be the primary field? Would you store NULL in the other one _always_? It's anything, but not elegant.
Other way is to create two tables.

alex_b’s picture

alex_b commented

#17: $param it is then : )

alex_b’s picture

alex_b commented
StatusFileSize
new 433388-19_exportable_mappings.patch17.35 KB

- Cleaned up some language
- Added doc to README for explaining how to export
- Simplified 'status' (revert) readings.
- removed the ability to export per node mappings.

I think this is very close. After the remaining issues from #12 are addressed, we should commit this.

However, the normal/default/override UI in this patch make it very clear that we need to follow up with #542260: Allow definition of multiple mappings, pick 'active' mapping once this patch is committed.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
StatusFileSize
new 433388-20_exportable_mappings.patch17.66 KB

#19: thanks for updating the patch!

What i did in that patch below in addition:
- when you export a per-node mapping, the warning appeared in the textarea, without any highlighting. I replaced it to drupal_set_message('foo', 'warning'), i assume this is the good way to show such messages to the user
- there was a broken /map/override link on the content-type mapping page, fixed, this should not appear there. Also i fixed the text there.
- revert functionality is fixed. you can only revert per-content-type mapping (but you can do that also when you are on a node/nid page and watching a per-content-type mapping, tricky :) )
- there was an issue when an exported mapping is on the UI and someone added a new mapping, the exported one wasn't merged into the new one and it started with an empty mapping. fixed
- put link to the README.txt at export to make it easier to access to the README, just as simpletest does it
- fixed typo in the README, missing )

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Needs work » Needs review

Hm, and after these, i think this needs review.

alex_b’s picture

alex_b commented
StatusFileSize
new 433388-21_exportable_mappings.patch17.8 KB

- Fixed call to drupal_write_record() in add function
- Fixed 'revert' string (minor)
- Fixed URL construction by adding a str_replace - str_replace('_', '-', $feed_node->type)

There is a remaining problem of confusion between 'override content type settings' and 'override default settings'. We should address this with #542260: Allow definition of multiple mappings, pick 'active' mapping though.

RTBC from my point of view, commit at your discretion, please.

Nice work.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 433388-22_exportable_mappings.patch17.7 KB

I did a little fix, removed an unneccessary variable assignment in add function.
Attached the final patch.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Reviewed & tested by the community » Fixed

Committed.
I wonder how many people will complain about the added hard dependency ;)

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented

A serious bug was fixed after #23 was committed.
Replaced

$map = (array) array_pop(ctools_export_load_object('feedapi_mapper', 'conditions', array('param' => $param)));
if (is_array($map)) {

To:

$map = (array) array_pop(ctools_export_load_object('feedapi_mapper', 'conditions', array('param' => $param)));
if (!empty($map)) {

Obvious, when i cast to array, is_array will be surely TRUE. Damn. Fixed. Also added 'ctools' to all the test setUp functions.

aron novak’s picture

aron novak
Primary language Hungarian
Location Hungary, Budapest
commented
Status: Fixed » Closed (fixed)
rickvug’s picture

rickvug commented
Issue tags: +CTools exportables

tagging for reference.