I was hoping to allow the exporting of noderelationship settings to a feature (using the feature module), and features already has ctool export functionality backed in, so I was looking into how to get noderelationships to work with ctools export.

And I came up with a little module that only does a small hook_schema_alter

// $Id: $
/*
 * Implementation of hook_schema_alter
 *
 * adds export functionality to noderelationships_settings
 * requires ctool patch http://drupal.org/files/issues/export_key_as_array.patch
 * from http://drupal.org/node/660786
 */
function noderelationships_exports_schema_alter(&$schema) {
  $schema['noderelationships_settings']['export'] = array(
    'key' => array('type_name','field_name'),
    'identifier' => 'nodereference_default_setting',
    'default hook' => 'nodereference_default_settings',
  );

}

it does also require a ctools patch #660786: Allowing export key to be an array as well as a string but I wanted to post it here before I built the module, incase this is something you would like to roll in.

All it does is allow other modules (like features) to export out the settings from the noderelationships_settings and then to implement them later on (if say they change in the db). but I think it is a big add for the small amount of code.

also as there is no code doing the export and import you do not have to have ctool as a dep, but if some one want to implement exporting and importing it will be there waiting from them

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markus_petrux’s picture

Status: Active » Postponed

I would be glad to add this feature to Node Relationships package itself. It could be a separate file that's loaded if the required modules are present from noderelationships_init().

I'm marking this postponed until the CTools issue is resolved. Please, let me know when that happens.

I have never seen this feature, so I would appreciate if you could describe what is exactly needed to use it, or a link to learn more. I would update the README.txt of the package and the project page as well to let other users know about it.

Overall, it looks pretty useful. Thanks! :)

e2thex’s picture

i will let you know if/when the ctools patch is accepted

This is the blog post that I found the most useful
http://civicactions.com/blog/2009/jul/24/using_chaos_tools_module_create...

there is a small patch need for features, but after it is in then this would would work out of the box with features.

redben’s picture

subscribing

rickvug’s picture

Issue tags: +CTools exportables

tagging.

e2thex’s picture

FileSize
1.11 KB

I am posting the patch I am using at the moment

e2thex’s picture

FileSize
1.11 KB

Ok so my last attempt and ctools export integration appears to have not touch all of the bases.

So i have made another attempt. Unfortunately, this one is much more invasive.

It appears that other modules are already using the key field.I am antisipating that making it an array will not be accepted. So First step was to add an export_key field to the noderelationships_settings table. and populate it on settings_save with $type_name__$field_name.

This was easy enough. But then the large question comes about on how is this useful. The main idea about exporting is so the settings can be in code, as a default. I am using the features module for this purpose.

So I had to add functionality to noderelationships to use settings that are in code.

To do this I interjected in the settings_list function, a call that pulls those settings out of the code into a temp table and then returns the results from that table as well as the results from the normal setting table (with the real table winning if there is a conflict).

Then I add two other ctool export hooks, that list all of the exportable fields and create the exported code.

I have attached the patch. I would expect that I will be hitting the patch again over the next few days.

I know this it contains alot more changes, but it does allow settings to live in code. :)

e2thex’s picture

FileSize
7.18 KB

Ok so one of the issues I was having with features is that it was not able to fine the entry that were in code for exporting, I am going to bring this up over there but for the moment I have change the code so that the defaults are put in to the noderelationships_settings table.

markus_petrux’s picture

hmm... I think it shouldn't be necessary to alter the DB design of the module just to integrate with another module that provides import/export features.

Note that the plain alternative it to just back up the module table on the source system, and restore on the target system.

So, IMHO, anything here should be transparent to the original module design, and optional.

irakli’s picture

FileSize
7.19 KB

@markus_petrux,

I will have to respectfully disagree. I would 100% agree with you, if CTools export/import was just for migrating things from one system to another. But it is not.

The most important use-case for Ctools exportables (and Features) is the ability to capture configuration in code. Before CTools/Features, a lot of configuration was in the database and it was hard to version-control those or package configuration with a module, a Feature or a distribution. Now there's an increasing understanding in the Drupal community that configuration is an integral part of functionality and needs to be shipped with a module. Being able to capture pre-configured node type in my code (incl. node relationship configuration for that node type) allows me to release a very useful functionality. And if the configuration is CTools Exportable-compatible, when I update my module, I will be able to release changed configuration as code!!!

This is why CTools and Features are not just another modules and are extremely important. Without exaggeration: they are game-changers.

It would be very useful if node relationship is compatible with CTool exportables.

P.S. I found some bugs in the version of the patch Erik submitted, specifically: not all table names were properly enclosed with "{}", so submitting an updated patch.

markus_petrux’s picture

You are still able to provide configuration changes in code using hook_update_N() in your own module, and if that module makes use of other modules, you can also implement configuration changes for them.

I can understand that CTools/Features could be a game changer, though. So, I think I'll leave this open until these features in CTools/Features are committed and tested. Otherwise, I think there's potential noise I could get, when I do not fully understand how it works, mostly because of lack of time right now.

One concern in regards to the patch, is that is adds a new column to the NR table, just to add redundant data. I do not like this.

e2thex’s picture

I would agree that adding redundant data is something that one should aviod. But what we are really adding is a identifier. While it is pulled from the other fields this is to give it meaning. But is main purpose is as a setting id.

So one could look at it as a new piece of info.

When you get bandwidth if you would like we can talk though the patch and what I am at least trying to do, and where I felt constrained by ctools export.

that0n3guy’s picture

subscribing,
features module = sweet
noderelationships module = sweet....

so: features module = noderelationships modules.... nope (I guess not all equations work, but they both still = sweet)

irakli’s picture

FileSize
8.07 KB

A revised patch with significantly improved performance (no temp tables and some caching) is attached.

irakli’s picture

FileSize
9.05 KB

Please disregard the previous one. Here's the properly generated one that works with drush make.

P.S. You need to run drupal_cache_clear_all() after applying this patch.

irakli’s picture

FileSize
10.39 KB

Further updated, optimized and cleaned-up version of the patch is attached.

@Markus,

new patch gets rid of the extra table (in both its temporary and permanent forms), so hopefully that solves the major concern you had accepting the patch?

The only change to the database structure, at this point, is the introduction of the export_key synthetic identifier. Changing that is beyond our abilities because it's a hard requirement coming all the way from CTools.

Please review the patch and let us know if there's anything further we can help with. We would really like to see NodeRelationships become exportable, since NR is incredibly useful, a true must-have module, and making it exportable is essential to its viability for any Drupal-based product development (most of which do or should use CTools/Features for settings versioning).

Thank you!

irakli’s picture

FileSize
7.54 KB

Further refactored code with some cleanups and a bunch of extra code removed.

markus_petrux’s picture

Could we add an autoincremented ID to the table (new primary key), and use that as "export_key" as in this patch (though it could simply be cid, config_id) rather than using a combination of values in other columns?

irakli’s picture

Markus,

thank you for your response.

There're two fundamental problems with using an auto-incremented field:

1. Since export_key is used to export from one site and import into a completely different site (and therefore database) export_key must be unique not just within any one database, but globally. Auto-increment fields can not possibly ensure that, since they are unique only within a table of a database.

If we use auto-increment fields a noderelationship exported from one site (e.g. development) will inevitably conflict with id's in the destination website.

2. Features user interface uses export_keys. When export_key is something like: "article__field_author" it is much clearer what the setting stands for than if the export_key was, say, 12312331234, because it's clear what the former depicts and not so much in the case of the latter.

I very well understand your concern regarding export_key field. It looks like it violates the no-data-duplication rule of the database normalization theory. But does it really?

This is how you could, also, look at the whole thing:

export_key is not really an aggregate of "type_name" and "field_name" columns. It has its own meaning: export_key is the global unique identifier of the record.

It just so happens that in our implementation we construct it using type_name and field_name values, partially for the lack of a better solution, right now. We could have used another algorithm to derive export_key. The only reason we did not do so, is because the one we used was the simplest algorithm that still made sense. Yet the inherent meaning of export_key is not to be the sum of already existing fields.

Since the field has its own, independent meaning, it also has got the right to exist on its own without violating database normalization rules.

markus_petrux’s picture

hmmm... the problem is that primary key here is based on type_name, relation_type, related_type, field_name. What if that was changed between versions?

So... maybe we could generate a random value for export keys? Using something unique to the site plus current timestamp?

PS: Thanks a lot for working on this. I think this feature is really great to have.

irakli’s picture

Random might work, except it would make export/import user interfaces not legible in Features and CTools since those show export_keys. For instance, if you need to export node relationship for the Author field of Article node type - how would you know that it corresponds to noderelationship with the export_key of 1098398? For the lack of anything else export_key serves as both "human readable" and "machine readable" name of a relationship setting. While random key might work for machine readable, it would be useless for human-readable use-case.

How about this. Maybe we should extract the logic that constructs export_key from a node relationship object into a separate function, thus making the maintenance easier? You are correct: if you change fields you will have to update code for export_key generation as well. However you won't necessarily have to update legacy keys, since they are probably still globally unique, so that's good news. But by having the logic of export_key generation in a separate visible place, under a well-documented separate function, it will probably greatly reduce the risk of missing it, in case of future change?

Thank you for your thoughtful responses and attention/time you give to this issue.

jhedstrom’s picture

irakli, is this patch currently working with features export, and if so, did you need to do anything in addition to this patch? I'm particularly confused by the hook in the patch, hook_table_name_to_hook_code, as I can't find any reference to such a hook.

irakli’s picture

@jhedstrom,

yes, it's a working patch. hook_table_name_to_hook_code is a CTools Exportable Objects hook. Feature uses CTools for export/import. This is how Advanced Help that comes with the CTools module, describes the hook:

-- to hook code callback
Function used to generate an export for the bulk export process. This is only necessary if the export is more complicated than simply listing the fields. Defaults to $module . '_' . $table . '_to_hook_code'.
jhedstrom’s picture

@irakli

Hmm, ctools is picking them up just fine from the schema, but they aren't being exposed to features. From what I can tell, its the lack of a features_api hook, and for whatever reason, this function isn't being generated dynamically in features.ctools.inc file. I'll continue to look into this. Thanks for your work on getting this going.

jhedstrom’s picture

I found the problem. There was a typo in the .install file, referring to the module owner as "nodereleationships". Attachted patch is the same as the one in #16 with the spelling changed to 'noderelationships'. Shows up on features now and properly exports the settings.

rapsli’s picture

What is the status here? Is this gonna get into the module? I do think support for features is a gameplayer!

iamer’s picture

I tried this patch and had a problem that it created non-unique export keys.

I have a photo content type that is referenced by multiple other content types using a shared cck nodereference field. For the export keys to be unique I changed the patch to include more items like this : $nodetype .'__'. $relation_type . '__' . $related_type . '__' . $field_name

rapsli’s picture

Version: 6.x-1.2 » 6.x-1.6
FileSize
6.87 KB

this patch should work for the 1.6 version.

There was only a problem with conflicting functions (noderelationsips_update_6003).

When is this patch getting into the module?

markus_petrux’s picture

@rapsli #27: Sadly, I do not have the time to test this new feature because it requires modules that I do not use. So, I would prefer to get as much feedback as possible from other users trying this feature. For example, see #26. This is also related to my concerns on how the key is generated (see #19).

markus_petrux’s picture

Status: Postponed » Needs work

I think "needs work" better reflects the status of the issue.

rapsli’s picture

I understand if you are not interested in using this feature. To get the community better involved in testing:
- Create a downloadable module of the current version (there are a lot of people who don't know how to do that patching stuff)
- invite people on the project page to help testing

an other alternative would be to create a new branche with just this feature. We would then see, how many people are actually using it, which could then also be a indication on the stability?

What do you think?

markus_petrux’s picture

I just released a new version yesterday and AFAICT there are no big things pending, so it looks like a good moment to introduce such a new feature, however the issue with the export key would have to be fixed, and then, I would prefer a minimum level of feedback from other users before doing commits.

irakli’s picture

@markus_petrux,

i can create an updated patch per @iamer's suggestion. Do you want it created against DRUPAL-6--1 branch or the release tag? I assume it may be one and the same, for now, since release happened just today.

Thanks,

Irakli

KarenS’s picture

OK, add me to the list of people who need this feature. I don't have time to test a patch right now, but can make time in the near future if someone wants to get the patch updated.

markus_petrux’s picture

@irakli: Oops, sorry, I think I missed your comment. Patch would have to be against the DRUPAL-6--1 branch, so that people can try it with the latest 6.x-1-x-dev snapshot. Thanks.

@KarenS et all: Once we can get a bit of feedback, I think we could commit and proceed from there.

markdorison’s picture

subscribe

q0rban’s picture

subscribe

q0rban’s picture

Quick read through the patch shows overall this looks pretty good. One concern:

+/**
+ * Add export key 
+ */
+function noderelationships_update_6004() {
+  $table = 'noderelationships_settings';
+  $field = 'export_key';
+  $schema =  noderelationships_schema();
+  $spec = $schema[$table]['fields'][$field];
+   db_add_field($return,$table, $field, $spec);
+   db_query("UPDATE {noderelationships_settings} SET export_key = concat(type_name, '__', field_name)");
+  return array();
+}

I have a feeling that query won't work for pgsql. I would change it to this:

  global $db_type;
  if ($db_type == 'mysql' || $db_type == 'mysqli') {
    $ret[] = update_sql("UPDATE {noderelationships_settings} SET export_key = concat(type_name, '__', field_name)");
  }
  else {
    $ret[] = update_sql("UPDATE {noderelationships_settings} SET export_key = type_name||'__'||field_name");
  }
q0rban’s picture

Assigned: Unassigned » q0rban

Will be working on an updated patch.

q0rban’s picture

Also..

    $schema =  noderelationships_schema();

Calling the schema like this in an update_hook is a bad idea. Consider the case where the schema may change in the future, but a user hasn't already run this update.

q0rban’s picture

Assigned: q0rban » Unassigned
Status: Needs work » Needs review
FileSize
6.49 KB

Ok, here's an updated version that works with the latest version of Features. Some changes to the original patch:

- Documentation and commenting throughout.
- Greatly simplified logic in noderelationships_default_settings_sync().
- noderelationships_default_settings_sync() now uses ctools_export_load_object() to ensure that files are included (Required for recent versions of Features).
- noderelationships_default_settings_sync() verifies that the field instance exists before writing the record.
- The settings field in the noderelationships_settings table now has serialize set to TRUE, which simplifies drupal_write_record().
- The base name for include files is now 'noderelationships' instead of 'default_node_relationships'.
- noderelationships_noderelationships_settings_list() will return an empty array if no options are found, so that features won't give you errors if there isn't anything to export.
- noderelationships_update_6004() doesn't pull from noderelationships_schema() anymore.
- noderelationships_update_6004() checks $db_type before creating the new export_key.
- noderelationships_update_6004() now populates a $ret array of queries.

q0rban’s picture

Updated patch now automatically adds the noderelationships settings for associated exported node types and CCK fields.

markus_petrux’s picture

The primary key of the settings table is type_name, relation_type, related_type, field_name. Shouldn't it be reflected to the export key? What if more than one record in the settings table has the same export key?

Other than that, the patch looks good to me. Thanks a lot for working on it!

q0rban’s picture

Status: Needs review » Needs work

Thanks, I will work on a revised patch, as I hadn't considered the case where a field instance might be both a node ref and a back ref.

e2thex’s picture

@q0rban I was reviewing the change you made to the patch, and it looks like you removed all of the caching in the noderelationships_default_settings_sync() function. I think that is going to get called quite a bit, is there a reason why you took it all out?

q0rban’s picture

e2thex, IIRC, ctools does all that caching for you. :)

e2thex’s picture

FileSize
7.14 KB

OK this attached patch is a continuation of #41's but the export key is a concat of all the primary key items.

I am still looking at the caching but my first look gave the impression that ctools does indeed handle it.

I still see one large issue: because node the entry in the node relationships_settings is delete when there are no settings, if a user tries to override a exported default, to have no values, as soon as it is queried again the default show back up. So in effect on can not turn off a node relationship if there is a default.

We might need to track which defaults have been imported

jaydub’s picture

subscribe

markus_petrux’s picture

Re: "I still see one large issue: because node the entry in the node relationships_settings is delete when there are no settings, if a user tries to override a exported default, to have no values, as soon as it is queried again the default show back up. So in effect on can not turn off a node relationship if there is a default."

I'm not using CTools export features myself so I do not quite understand that, I think. Defaults in these settings mean there are no features enabled for the node reference field. The module removes those records to optimize itself. I cannot see if that's a problem for CTools export features.

Other than that, the patch now solves the issue about the primary key which was of my concern, so it would be good for me to go. That being said, better try to resolve any remaining issue before committing. If someone is interested in this feature, please report back about the patch and these pending doubts. The most feedback we get, the better for everyone. Thanks!

jaydub’s picture

I tried the patch in #46 and the problem I get is this: when enable a feature with Node Relationships exported definitions or if I delete any rows in noderelationships_settings table ( in order to force a rebuild from the export file ) the function noderelationships_default_settings_sync() is run for every defined content type (in my case 13). This function attempts to write a record to noderelationships_settings table for each record in the export file. However because the sync function is called separately for each content type, once the initial records are written from the export file to the DB, subsequent calls to the sync function result in DB insert failures since the rows now exist in noderelationships_settings.

I don't think my solution is the ultimate fix but for now I've revised the patch to at least first see if a row exists for a given export_key and if so, an update is run via drupal_write_record() rather than an insert.

e2thex’s picture

seems to me that if you $exist is true we should not do anything. and only do something if does not exists.

But the question is why is $record->in_code_only not catching this?

brandonvalentine’s picture

Reading through this issue and the massive number of attempted patches it seems to me wholly unnecessary that node relationships use its own table in the first place. Nothing stored in noderelationships_settings cannot just be stored in the variables table like any number of other Drupal modules storing content type specific settings. Storing it there would make it easy to export the settings use Strongarm, which is a pretty standard part of most folks Features workflow. Why hack around half the system when you don't need to keep the noderelationships settings in a separate table in the first place?

markus_petrux’s picture

The reason for not using the variables table was performance. 1) The variables table is cached, yes, but the whole cached block is transferred from the DB server for each page, and it might be a huge amount of data already to add more stuff that can be stored elsewhere and read only when necessary. 2) I wrote this module for a site that has a lot of content types, and it was necessary to think about performance everywhere. Using the variable table would have impacted every single page of the site, to read all the information about all node relationships, where only a few or none would have been necessary.

e2thex’s picture

So The piece that makes this so complicated is the use of the where clause in the data collection function,

If this flexibility was trimed to say using conditions then the ctools_export_load_object could be used
http://drupalcontrib.org/api/function/ctools_export_load_object/6

I do not see anything in the code currently that would make this prohibited but it I do not know what is planed down the road.

phayes’s picture

Instead of going whole-hog on settings-in-code-via-ctools (which is better in the long run), is there interest from the module maintainer in a patch that would add features support via "faux-exportable" functionality? It would be far less invasive, and would just allow the database records to be passed along with a feature.

I'd be willing to code this patch immediately if there is interest. (With the recognition the the ctools route is better in the long run).

markus_petrux’s picture

As far as I am concerned, there are no more planned features that may affect the primary key of the settings, which is the critical part of this issue, I think.

I'm not sure where we are now. What I'm waiting is confirmation to commit last patch. Please, see #48 above. My concern about the primary key was resolved, so I need feedback from someone who can confirm the patch works. Then I think it will be easier to proceed from there. Once the patch is applied to CVS, we can wait a little to see if any new issue arises, and when everything looks good, a new release can be created with this feature included.

Is the issue status correct as it is? TBH, I haven't tested, so I cannot really tell. :-|

phayes’s picture

Okay. I'll try applying the patch #49 above and report back.

phayes’s picture

Status: Needs work » Reviewed & tested by the community

I can confirm that the latest patch works

q0rban’s picture

Status: Reviewed & tested by the community » Needs work

Based on the comment in #46, this patch is "needs work". The method of syncing what is in code with what is in the database is a Bad Idea (tm), as we can see by all the little problems people are running into. We need to rework this to use the method described in #53, basically to allow for conditional loading of rows directly via ctools_export_load_object. Then we need to allow for deletions. Finally we need to allow for reverting back to what is in code without deleting.

irakli’s picture

Maybe we should do what q0rban and I did for workflow?

Create new branch, export it to Github, do a clean, fundamental exportability update, rather than "quick patch", then Markus can review and after some revisions, hopefully accept, import as new branch in CVS and switch node_relationships recommended version to that branch?

Seems like we are beating the same broken drum while a step back, look at a bigger picture and one last blow at the problem could bring the solution home?

I am down with helping out with this, obviously.

P.S. GitHub only enters the picture because it is SO MUCH easier to iterate over code over there, instead of rolling and re-rolling patches.

tjwallace’s picture

After trying the patch from #49 (checking off each setting in my feature and exporting it), I've noticed that on a fresh database the noderelationship settings are not being inserted into the database as you would expect. $records = ctools_export_load_object('noderelationships_settings'); (from noderelationships.inc) only returns settings that are in the database, not in code... Has anyone had this problem or am I forgetting some step?

EDIT:
I found the problem, I forgot to properly implement HOOK_ctools_plugin_api() (return array("version" => 1) for noderelationships).

bleen’s picture

subscribing

codi’s picture

Has any work been done on this in the last week or two? Does some testing need to happen still?

kmadel’s picture

I successfully tested the patch from #49.

ericduran’s picture

I don't think patch #49 takes the right approach.

ctools_export_load_object allows for conditional loading, so I don't see why there needs to be a sync to the database.

I just looked at the noderelationships modules for a couple of minutes, I'll investigate this further later when I have more time but it looks like we can just modified the noderelationships_settings_list function to take an array with key => value pair of the where conditions. That way if ctools in enabled we can just pass the array straight to ctools and it'll return the correct value for us, from the database and the export settings and if not we can do a regular query.

Something like this :

I didn't test the below code :-p, but you can get the idea.
This should allow for proper loading from export and db.

<?php

function noderelationships_settings_list($args) {
  
 if (module_exists('ctools')) {
   $result =  ctools_export_load_object('noderelationships_settings', 'conditions', $args);
    // Process the result
  }
  else {
    $query = 'SELECT * FROM {noderelationships_settings}';
    $schema = drupal_get_schema('noderelationships_settings');
    foreach ($args as $key => $value) {
       if (isset($schema['fields'][$key])) {
        $conditions[] = "$key = " . db_type_placeholder($schema['fields'][$key]['type']);
        $query_args[] = $value;
      }
    }
  }

  // Make a string out of the conditions.
  if ($conditions) {
    $query .= " WHERE " . implode(' AND ', $conditions);
  }

  $result = db_query($query, $query_args);
  // Process the result

}
?>

Then all we have to do is change every call to noderelationships_settings_list to pass in the correct argument something like

<?php
noderelationships_settings_list(array("type_name" => $nodetype);
?>

I'll try to implement this when I get some time, but I think that should work. :-)

e2thex’s picture

I think that should work as well, as long as the maintainer is willing to change the noderelationships_settings_list calls and to accept that in the future they will have to work with with condition instead of where clauses.

There might still be an issue with having no record in the database be a valid state, as the code would most likely kick in, but that could be address relatively easily i think if we went down this path.

For what its worth I think this would be a nice solution

ericduran’s picture

Assigned: Unassigned » ericduran

I'm going to work on this for a little bit now.

ericduran’s picture

Assigned: ericduran » Unassigned

Change of priorities not working on this now :( but I'm still confident that what I posted above should work.

DamienMcKenna’s picture

A generic approach to this would help several modules: #924236: Primary key auto-detection, support for compound primary keys

bmnic’s picture

Subscribing

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
7.38 KB

An alternative patch written from scratch, could use some eyeballs.

DamienMcKenna’s picture

A slight re-do of the patch, this time using just implode/explode on the dataset rather than base64_encoding() the serialized keys array. This makes it much simpler to handle.

DamienMcKenna’s picture

Small bug in the last patch.

DamienMcKenna’s picture

A slight cleanup of the previous patches: the key values are now sorted in the order of the primary key fields rather than being run through ksort(), got rid of the unnecessary $export['keys'] value in hook_schema(), renamed and some slight comment changes.

DamienMcKenna’s picture

Bug fixes, hopefully the last patch ;)

DamienMcKenna’s picture

Status: Needs work » Needs review

Discovered a problem, I need a better "load callback" after all. A new patch will be on its way tomorrow.

DamienMcKenna’s picture

Status: Needs review » Needs work
Deciphered’s picture

Status: Needs review » Needs work

Just ran a quick test with this patch, and it's definitely not ready. It's showing up as overridden at all times.

Planning on looking into it, but as you mentioned a new patch I figure I'm probably duplicating efforts, so I'll wait a little bit.

Thanks for the great work done so far.

DamienMcKenna’s picture

A small update that fixes the table name preprocessing - this doesn't fix the active problem, it just tidies up what's already there.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
8.88 KB

Try this. It inserts the default records after doing drupal_flush_all_caches().

DamienMcKenna’s picture

Yes, that probably needs some more work, but I've been spinning my wheels on it for too long and need to move on. Could someone please take a look?

Deciphered’s picture

Will give it a test, and see if I can fix anything, but the small amount of time I put into digging through the code I was unable to really figure out what was going on, but I'll do what I can.

jhedstrom’s picture

The patch in #79 doesn't seem to work. It seems that the update hook from #41 (which works) has gone missing.

hefox’s picture

This is a combination of 64 and 79, cause I really dislike the idea of defaults stored in the database.

Anyhow, it required one patch to ctools to allow it to work in the vain of #924236: Primary key auto-detection, support for compound primary keys, but it is by far from complete (but should be enough to get this working for what it needs to do for noderelationships_settings_list).

It changes noderelationships_settings_list (and noderelationships_settings_delete due to how it was being used) to accept an array of conditions. When ctools is enabled, noderelationships_settings_list uses ctools function to grab the info, else it fallsback to a database query.

Anything exported with previous patch should still work.

DamienMcKenna’s picture

@hefox: Thanks for working on the patch, I'll take a look.

One reason I gave up was doing more reviewing on NodeRelationships indicated that it executes queries against the settings table, which clearly would not work with data loaded via a defaults structure. I had thought of just dumping the whole exportables patch and just writing a patch to move all settings to variables, then updating the variables to use some extra conditions ("WHERE x = variable_get('noderelationships_type_field_yadda')"), which would simplify the whole structure; of course then there wouldn't be a need for a CTools patch.

hefox’s picture

As far as noderef settings go, if I was to redo node relationships I'd just store it with the field widget settings (there's a hook that cck has now to do add form/saving settings for both field settings and field instance [widget] settings) (which again would negative the patch at least for 'noderef' as then the settings would be exported with the -- not using backref so not sure if if that's possible).

So instead of configuring the node references in mass, that'd be configured from the field settings, which in some respect makes more sense. (though the mass configuration is nice and likely could be adapted to also storing in field settings instead of noderelationship_settings table, as there's already a presidence for that). However, that'd mean a more painful migration, particularly for those already using features (as an update hook that would modify the content fields table would then loose those settings next revert on the feature, unless they know to update the feature). It'd also make exporting more easy, and it's exported with the field.

The idea is sounding better and better though, but I personally do not want to re-write the user interface to support this. Hm.

Deciphered’s picture

I second that approach, it's the approach that I've taken with Wysiwyg Fields, and the approach that I plan to take with FileField Paths 2.x.

hefox’s picture

Looking it over, I see that it might be possible to do backref as a field settings, but it's going to be uglier.

I suspect the solution for backref if to store the settings in the content fields settings, but not do the ui on the field settings form. Either way, gonna be weird. However, if changing to one way, should change both likely.

jhedstrom’s picture

Status: Needs review » Needs work
FileSize
14.43 KB

Patch in #83 works, in that noderelationship setting exported to code are coming through. However, it still shows the feature as perpetually overridden.. It seems that an overridden state is being detected because the call to features_get_signature('normal'...) is failing to return the expected code. I'm digging into this a bit more.

There is also a fatal error in the patch from #83 when calling noderelationships_settings_delete() without first including noderelationships.inc--a fix is attached, aside from that, this is the same patch.

jhedstrom’s picture

Digging into this a little more, I've traced the issue with the features always being overridden back to the noderelationships_ctools_export_to_hook_code() function. When features is detecting overrides, it loads 2 versions of the exportable, one using the 'normal' method, and the other using the 'default' method. When features loads the normal version, ctools calls the aforementioned hook, which returns false if the noderelationship settings to not exist in the database.

Given all this, I like the new idea of simply moving away from depending on ctools, and going with field settings instead.

patricksettle’s picture

There's an issue with the patch in #88. in noderelationships.inc the function noderelationships_settings_list() loads ctools_export_load_object but then doesn't check to see if the settings form has been modified on save, preventing any chance of overriding the settings, (e.g. node reference extras). Haven't gotten a chance to explore a solution.

Open Social’s picture

The issue with the overridden state is because the function noderelationships_settings_features_export_render does not exist, I am trying to find what needs to be in it.

I am trying to fix the issue of #88

Open Social’s picture

Found it.
Here is the missing function. Add it to noderelationships.module


function noderelationships_settings_features_export_render($module_name = '', $data) {
  ctools_include('export');
  $component = 'noderelationships_settings';
  $schema = ctools_export_get_schema($component);
  $objects = ctools_export_load_object($component);

  $code = array();
  $code[] = '  $export = array();';
  $code[] = '';
  foreach ($data as $machine_name) {
    // The object to be exported.
    if ($object = $objects[$machine_name]) {

      $additions = array(
      	'__keys' => _noderelationships_ctools_export_primary_keys($object),
      	'__compound_key' => $machine_name,
      );


      // Code.
      $identifier = $schema['export']['identifier'];
      $code[] = ctools_export_object($component, $object, '  ', $identifier, $additions);
      $code[] = '  $export[\''. $machine_name .'\'] = $'. $identifier .';';
      $code[] = '';
    }
  }
  $code[] = '  return $export;';
  $code = implode("\n", $code);
	$return = array($schema['export']['default hook'] => $code);
  return $return;
}
Open Social’s picture

o well, not really.

I can not edit the settings now.

We check further

Open Social’s picture


function noderelationships_settings_features_export_render($module_name = '', $data) {
  ctools_include('export');
  $component = 'noderelationships_settings';
  $schema = ctools_export_get_schema($component);
  $objects = ctools_export_load_object($component);

	//dpm($objects);

  $code = array();
  $code[] = '  $export = array();';
  $code[] = '';
  foreach ($data as $machine_name) {
    // The object to be exported.
    if ($object = $objects[$machine_name]) {

      $additions = array(
      	'__keys' => _noderelationships_ctools_export_primary_keys($object),
      	'__compound_key' => $machine_name,
      );
      
  		$query = db_query("SELECT settings FROM {noderelationships_settings} WHERE type_name='%s' AND relation_type='%s' AND related_type='%s' AND field_name='%s'", 
  		array($object->type_name, $object->relation_type, $object->related_type, $object->field_name));
		  while ($r = db_fetch_object($query)) {
				$settings = unserialize($r->settings);
		  }
			
			$object->settings = $settings;
      // Code.
      $identifier = $schema['export']['identifier'];
      $code[] = ctools_export_object($component, $object, '  ', $identifier, $additions);
      $code[] = '  $export[\''. $machine_name .'\'] = $'. $identifier .';';
      $code[] = '';
    }
  }
  $code[] = '  return $export;';
  $code = implode("\n", $code);
	$return = array($schema['export']['default hook'] => $code);
	//dpm($return);
  return $return;
}

/**
 * Implementation of hook_features_revert().
 */
function noderelationships_settings_features_revert($module_name = NULL) {
  $table = 'noderelationships_settings';
  $defaults = features_get_default($table, $module_name);
	foreach ($defaults as $key => $object) {
		$settings = serialize($object->settings);
 		$query = db_query("UPDATE {noderelationships_settings} SET settings='%s' WHERE type_name='%s' AND relation_type='%s' AND related_type='%s' AND field_name='%s'", 
 		array($settings, $object->type_name, $object->relation_type, $object->related_type, $object->field_name));
	}
}

Still one bug: You can not de-select a settings that is in a feature

Deciphered’s picture

goalgorilla,

It would be extremely helpful if you can roll a patch for this, if you're unsure how refer to http://drupal.org/node/446038/git-instructions, specifically under the header of 'Patching'. Once you've done that you can submit and set this issue to 'needs review' and hopefully push this issue forward.

Cheers,
Deciphered.

Open Social’s picture

Ok, I will,

My noderelationships module is heavenly patched because of other issues. I will git clone the project and supply a patch.

Open Social’s picture

This patch has the patch of jhedstrom (#88) in it.
You need to patch 6.x-1.6
It will add features (revert and correct checking) integration.

Deciphered’s picture

Status: Needs work » Needs review

Marking as 'needs review'.

dominikb1888’s picture

With the patch in #97 I am still facing the same issue as in #90. Are there any hints as towards solving this issue? I am using 6.x-1.6 and node reference extras and the following patch to show delete icons: http://drupal.org/files/issues/noderelationships.delete-842456-23.patch

Specifically I have the following problem: I have three node relationship fields on one content type. In the settings for node reference extras only the checkboxes for the last field are editable and checked as according to the exported feature. The first two neither show the exported options nor keep the settings in the form after submitting it.

dominikb1888’s picture

In addition to the previous post I am receiving the following PHP Notices:

Notice: Undefined property: stdClass::$__compound_key in ctools_export_load_object() (line 389 of /home/quickstart/websites/noderel.dev/profiles/openatrium/modules/contrib/ctools/includes/export.inc).
Notice: Undefined property: stdClass::$__compound_key in ctools_export_load_object() (line 393 of /home/quickstart/websites/noderel.dev/profiles/openatrium/modules/contrib/ctools/includes/export.inc).
Notice: Undefined property: stdClass::$__compound_key in ctools_export_load_object() (line 395 of /home/quickstart/websites/noderel.dev/profiles/openatrium/modules/contrib/ctools/includes/export.inc).

dominikb1888’s picture

I could track down my specific problem to the following lines in noderelationships.inc, line 25:

function noderelationships_settings_list($args = array()) {
  if (module_exists('ctools')) {
    $settings = ctools_export_load_object('noderelationships_settings', 'conditions', $args);
  }

When commenting out the call to ctools_export_load_object and relying on the handling of the settings table through noderelationships instead of ctools, everything works fine.

However, in no case any code gets exported to a feature.

drclaw’s picture

Title: ctool export intergration » ctools export intergration (features support)
Version: 6.x-1.6 » 6.x-1.x-dev
Issue summary: View changes
FileSize
16.17 KB

Hi all,

In case anyone is still looking for exportability through ctools/features, I've tweaked the patch from #97. If you've been using any of the above patches and have some noderelationships code partially exported already, you may need to do one of two things:

First, check your .info file and look for a line that looks like this:

features[noderelationships_settings][] = "module_name:noderef::field_name"

You should have one of these lines for every exported node relationship settings object in module_name.noderelationships.inc (hint: search for $noderelationship->__compound_key = 'module_name:noderef::field_name';

If that doesn't work, remove all noderelationships code from your feature and re-export. This would entail removing the module_name.noderelationships.inc file from the feature, as well as removing any noderelationships lines from your .info file. If you do this, you may also need to reconfigure your node relationship settings on each field prior to export, but after removing it from your feature.

Hope this is useful for someone!
drclaw

drclaw’s picture

Title: ctool export intergration » ctools export intergration (features support)
Version: 6.x-1.6 » 6.x-1.x-dev
Issue summary: View changes
FileSize
16.17 KB

Hi all,

In case anyone is still looking for exportability through ctools/features, I've tweaked the patch from #97. If you've been using any of the above patches and have some noderelationships code partially exported already, you may need to do one of two things:

First, check your .info file and look for a line that looks like this:

features[noderelationships_settings][] = "module_name:noderef::field_name"

You should have one of these lines for every exported node relationship settings object in module_name.noderelationships.inc (hint: search for $noderelationship->__compound_key = 'module_name:noderef::field_name';

If that doesn't work, remove all noderelationships code from your feature and re-export. This would entail removing the module_name.noderelationships.inc file from the feature, as well as removing any noderelationships lines from your .info file. If you do this, you may also need to reconfigure your node relationship settings on each field prior to export, but after removing it from your feature.

Hope this is useful for someone!
drclaw

drclaw’s picture

Title: ctool export intergration » ctools export intergration (features support)
Version: 6.x-1.6 » 6.x-1.x-dev
Issue summary: View changes
FileSize
16.17 KB

Hi all,

In case anyone is still looking for exportability through ctools/features, I've tweaked the patch from #97. If you've been using any of the above patches and have some noderelationships code partially exported already, you may need to do one of two things:

First, check your .info file and look for a line that looks like this:

features[noderelationships_settings][] = "module_name:noderef::field_name"

You should have one of these lines for every exported node relationship settings object in module_name.noderelationships.inc (hint: search for $noderelationship->__compound_key = 'module_name:noderef::field_name';

If that doesn't work, remove all noderelationships code from your feature and re-export. This would entail removing the module_name.noderelationships.inc file from the feature, as well as removing any noderelationships lines from your .info file. If you do this, you may also need to reconfigure your node relationship settings on each field prior to export, but after removing it from your feature.

Hope this is useful for someone!
drclaw