UUID support for default value in taxonomy term reference field settings

Changing to bug report because I found out that there was already code for this inside uuid_features, but it was still targeting Drupal 6 inside the Drupal 7 version. ($node->taxonomy)

So what I did is:

• Remove the old code
• Make sure module 'polyfills' are always loaded
• Add new code that fixes this issue, partly based on rogical's code

What the code does:

• Detect any taxonomy term reference fields with defaults
• If the terms were already provided by another feature:
  • Add the defining feature as a dependency
• If the terms weren't provided yet:
  • Add those default terms to the feature
  • Pipe the terms so that they are properly handled by uuid_term implementations

Note that this still needs the latest patch from the related issue to work.
#2158049-7: Allow alter field variable during field export

Removed some more legacy code: uuid_term.features.inc was still checking for term reference fields, but those are handled by this patch now. (Field instances are exported along with the vocabulary dependency and thus trigger the taxonomy term reference code).

Sorry for the noise, but I keep finding 'leftover' code from D6.

• Made better use of export pipes
• Made sure all polyfills are available

I've applied the patch on this page from comment #8 and the patch from comment #7 on the referenced issue for features. (https://www.drupal.org/node/2158049#comment-8419075).

Unfortunately after applying them both I get the following error if I attempt to recreate an existing feature:
PHP Fatal error: Call to undefined function uuid_term_features_get_dependencies() in "MyDrupalRoot"\sites\default\modules\uuid_features\uuid_features.module on line 102

Am I missing a step here?

Or is there a different/better way for me to export the default value on a content type that is linked to a taxonomy term id? My main motivation is so that I can easily transfer this feature from one server to another where internal taxonomy_ids are different.

Same problem here: PHP Fatal error: Call to undefined function uuid_term_features_get_dependencies() when applying patch from #8 to 7.x-1.0-alpha4. Is there a nice solution for that problem?

I guess this patch needs a re-roll.

The patch removed uuid_terms_features_get_dependencies() without issues earlier. This means the latest version of UUID Features added a call to said function somewhere.

We created a patch which can solve the uuid issue for taxonomy term reference field type.

@Mover thank you very much for the re-roll, I just gave it a quick visual review. Mostly there are just coding standard nitpicks.

However, from a quick review it seems the patch from #8 has a more comprehensive feature-set e.g. dependency management. So I'm not sure how we'll proceed here.

1. +++ b/uuid_features.module
   @@ -496,3 +496,74 @@ function _uuid_features_file_field_import_file(&$file) {
   + * Alter the field instances right before they are cached into the database.
   

   Looks like a hook implementation - thus it should also use the documentation style: Implements hook_...().

2. +++ b/uuid_features.module
   @@ -496,3 +496,74 @@ function _uuid_features_file_field_import_file(&$file) {
   +  foreach($fields as $field_name => $field) {
   

   Could we mention what is done here.
   E.g. "Ensure the configured default value of a term reference field is properly set."

3. +++ b/uuid_features.module
   @@ -496,3 +496,74 @@ function _uuid_features_file_field_import_file(&$file) {
   +    if(!empty($field['default_value'])) {
   ...
   +        if(isset($value['uuid']) && isset($value['tid'])) {
   ...
   +          if($term) {
   +            if(empty($term->uuid)) {
   ...
   +      if(!empty($default_values)) {
   ...
   +      if($instance = features_field_instance_load($identifier)) {
   ...
   +            if(isset($value['tid']) && !isset($value['uuid'])) {
   +              if($term = taxonomy_term_load($value['tid'])) {
   +                if(empty($term->uuid)) {
   ...
   +          if(!empty($default_values)) {
   

   Expected 1 space after if to match the coding standards.

4. +++ b/uuid_features.module
   @@ -496,3 +496,74 @@ function _uuid_features_file_field_import_file(&$file) {
   +      foreach($field['default_value'] as $key => $value) {
   ...
   +          foreach($instance['default_value'] as $key => $value) {
   

   Expected 1 space after foreach to match the coding standards.

5. +++ b/uuid_features.module
   @@ -496,3 +496,74 @@ function _uuid_features_file_field_import_file(&$file) {
   +            } else {
   ...
   +                } else {
   

   The else should be on a new line to match the coding standards.

6. +++ b/uuid_features.module
   @@ -496,3 +496,74 @@ function _uuid_features_file_field_import_file(&$file) {
   +                  'tid' => $term->tid
   ...
   +                      'tid' => $term->tid
   

   Last element in an multiline array declaration should be trailed by a comma to match the coding standards.

7. +++ b/uuid_features.module
   @@ -496,3 +496,74 @@ function _uuid_features_file_field_import_file(&$file) {
   +    foreach ($export['features']['field_instance'] as $identifier) {
   

   Could we mention here what's going on.
   E.g. "Ensure the default values of taxonomy term reference fields are exported as uuids".

8. +++ b/uuid_features.module
   @@ -496,3 +496,74 @@ function _uuid_features_file_field_import_file(&$file) {
   +        if(!empty($instance['default_value']) && isset($instance['default_value'][0]['tid'])) {
   

   I think this condition can be simplyfied to:
   if(!empty($instance['default_value'][0]['tid']))

Thank you Peter Philipp and your review!
I have corrected some lines to match the coding standard. Regarding to some of your questions, please see the below answers:
1. I placed the two hook implementations in the uuid_features.module, there are a few reasons:

• The two hooks I am using which scope are global API and both of them can be implemented by any standard module, but for features components module's API seems only work for defined component.
• I think the feature component module makes me a little bit confused, such as naming, I don't know what does it depend on. but if i have to put this field_instace hook into one of component modules, we should put into a field_instance or field related component module, rather than taxonomy component module, but it can not be found in uuid_features. and I still found there is one taxonomy component hook was placed in this module file: uuid_features_features_pipe_taxonomy_alter(&$pipe, $data, $export)
• The uuid_features module need to be reorginized.

2. Use isset to validate tid, just want to avoid the notice in PHP logs.

Thanks,

@Mover thanks for the updated patch. I think there were misunderstandings about some parts of my review:
In 1. I don't question why the hooks are in the module file - since hooks usually belong there. The only thing is that if you implement a hook the function documentation should follow the coding / doxygen standards https://www.drupal.org/node/1354#drupal which says:

When implementing a hook, use a short summary of the form "Implements hook_menu().". Omit the parameter and return value documentation in this case.

Working like that allows others to quickly discover what hooks are used by a module by simply searching for "Implements hook_".
Regarding 8. there's simply no need to use both isset() and empty(), empty works like isset, but also checks if there's an empty / no value. So by switching to if(!empty($instance['default_value'][0]['tid'])) you'll check the most specific requirement right away, still without the danger of getting a notice.
You can even run the command below, where $instance isn't available at all and won't get a notice:

$r = empty($instance['default_value'][0]['tid']);
var_dump($r);

Try it with https://3v4l.org/kESBP

Hope this helps to clarify those points :)

Thank you Peter Philipp! I appreciate your comments very much! I have corrected them.

Re-roll to apply to 7.x-1.x with spelling correction in comments and comments wrapped.

Looks like the patch export correctly.

Marking as 'needs work' as when you 'review overrides' it only shows the 'tid'. It should show 'tid' and 'uid'. This happen when you have a field as part of a feature (without a default value), then add a default value to it.

I didn't get the issue, so I assume that is caused by the cache issue.

Re-roll.

Testing failed because there are no tests.

I am not convinced of this patch.

Observation: Noise in the stored default value

When saving the field edit form at admin/structure/types/manage/***/fields:
In taxonomy_autocomplete_validate(), the default value is replaced with the complete term object, and this is saved in the instance settings in the database. This contains the uuid, but a lot of other unnecessary stuff as well.

Problems in this patch

With this patch, on features export, the module would call field_update_instance() to overwrite the field instance config, setting the new format for the default value.
Exporting a feature should have no side effect on the field instance stored in the database.

Another problem is there seems to be a real bug in uuid_features_features_export_alter():
If there are two values in the default value, one of them has a uuid and the other has not.
Then $default_values will contain only one of the two item.
So one item will be lost when saving.

In uuid_features_features_export_alter(), if one of the values does not contain a 'uuid', the stored default value will be replaced with ['uuid' => .., 'tid' => ...], removing all the noise. However if the default value already contains a 'uuid', then it remains as it is.

For features this means that sometimes the export will contain the entire term, sometimes it will only contain uuid and tid for the default value.

Conclusion:
This patch adds unpredictability and possible side effects, in the worst case it may cause data loss.

What would be the correct way to do this?

The correct solution would be:

• Leave the field instance in the database as it is. (The problem with the noise stored in the db in taxonomy_autocomplete_validate() has to be fixed in core or elsewhere, but not here.)
• When exporting a field instance to features, the default value has to be rewritten to store only the uuid. The tid or anything from the node should not be exported. If there is currently no hook for this, a suitable hook should be added in features .
  The rewritten default value should only be written to the feature, not to the database of course!
• When reverting the feature, the term has to be loaded by uuid, and the tid has to be re-added to the config. I don't think we need to simulate the core situation where the entire term is saved in the default value..

What would be a cheaper workaround?

If the "bug" from previous message was not there, perhaps we could risk to save the default value in uuid_features_features_export_alter().
I don't think this should be committed to the uuid_features module, but it would then be more viable as a patch that people can apply until a real solution is found.

When exporting a field instance to features, the default value has to be rewritten to store only the uuid. The tid or anything from the node should not be exported. If there is currently no hook for this, a suitable hook should be added in features.

#2158049: Allow alter field variable during field export did attempt to introduce such hooks in features. It has its own problems, though.
Adding this to every feature seems repetitive.

Re-roll of #27.