Comments

my-family’s picture

I would also appreciate this.

mirom’s picture

Is there any progress with this?

vbouchet’s picture

I need this feature for a project and would start to work on a patch soon. I already did some basic testing but wanted to know first if the module's maintainers already have a solution in mind.

Right know my idea is the following:
- In conditional_fields_dependency_edit_form_submit(), update the $dependency['options']['value_form'] and $dependency['options']['value'] values to do the translation from the tid to the appropriate uuid.
- Update the conditional_fields_dependency_edit_form() to do the reverse translation and set the #default_value properly by translating the stored uuid to appropriate tid.
- Update the conditional_fields_load_dependencies() and conditional_fields_dependency_load() functions to update the $options array by translating the stored uuid to appropriate tid.

I would had a variable (let's say 'c_fields_uuid' to define if we must do the translation or no so even if people have uuid enabled, they are not forced to use conditional fields with the uuid support/translation).

Do you agree this plan? Do you have any suggestion?

vbouchet’s picture

Status: Active » Needs review
StatusFileSize
new2.61 KB

I changed my mind while doing the development. I finally thought that uuid support was needed for the import/export feature only.

I ended in updating the conditional_fields_features_export_render() and conditional_fields_features_rebuild() functions to do the translation from uuid to id while importing settings and from id to uuid while export settings.

To be sure this patch doesn't impact people using uuid but who don't want to export settings with uuid, I used a variable "c_fields_uuid" which must be set to TRUE before the translation happens.

Please note that for now it has only be tested with the "Inset value from widget" input mode so it only allows one value (but it can be easily match multiple values cases by using multiple conditions with the "OR" operator).

gambry’s picture

On my case scenario I don't have uuid (and/or if I do the uuids don't match) as the term is created during a Migrate process, so I can't use the solution above.

Why not using hooks alter on both `conditional_fields_features_export_render()` and `conditional_fields_features_rebuild()` ?
That would solve any problem and developers can integrate their solutions.

Thoughts?

vbouchet’s picture

I was also facing the same problem and decided to set the uuid in the xml used by the Migrate class so my terms always have the same uuid.

I am not sure to get the use of alter hooks. Can you describe it more please ?

gambry’s picture

I've actually found the hook I was talking (and for a while been looking) for.

With hook_conditional_fields_default_fields_alter() a developer can alter the conditional_fields data, both on exporting and reverting/rebuilding. So you could integrate UUID and any other bespoke logic without patching the Conditional Fields module.

However adding UUID integration could be useful for webmasters without PHP knowledge, so I would proceed with the patch.

Few feedback on the patch at #4:
- variable_get('c_fields_uuid', FALSE): you can remove this. Use module_exists('uuid').
- unset($dependee['options']['value'][0]['target_id']);: I would keep the target_id. It will be updated anyway if UUID module exists.
- You should also make sure an entity exists with the target_uuid, and defaulting back to the original target_id if not found.
- Conditional Fields 7.x-3.0-alpha2 (at least on my installation) doesn't use target_id but tid.

vbouchet’s picture

Hi gambry,

Thanks for you feedback. Maybe the existence of this hook is the reason why the patch has not been here earlier. However, as you said, I think it would be great to have a native support of uuid in conditional field.

- I did not wanted to use module_exists('uuid') because I assumed some developers using uuid and conditional field don't want to export using uuids. For now there is no admin interface to set this value.
- Keeping the target_id may make the feature in unstable state if the uuid does not match the target_id (I export a conditional field based on a taxonomy term with the tid 432 and uuid 54ab3-32ac4, I re-install my website or move my feature to another environment, the uuid will remains the same but the tid may be 555. The feature will be marked as overriden or to be reviewed).
- Based on my previous answer (and if we keep the current system) it is not possible if we don't keep target_id. Even if we keep the target_id, it is dangerous from my point of view as if the uuid does not exist, that probably means something wrong occured and the target_id may be not pointing the appropriate id.
- I guess you used the term_reference field and I used the entity_reference field. I should probably update the patch as this key should be dynamic.

gambry’s picture

StatusFileSize
new1.12 KB

I've been playing around and that hook doesn't do what I/we need.

I did not wanted to use module_exists('uuid') because I assumed some developers using uuid and conditional field don't want to export using uuids. For now there is no admin interface to set this value.

But using a variable makes the whole feature difficult to be used without programming knowledge.
May be checking if uuid_features exists and if the entity and bundle are on the uuid_features_* variable?

I'm personally not sure the audience for the patch should be the developer. Developers should have a more general and powerful way of altering the data.

And as apparently Features doesn't have a solution for updating the exported rendered data, I would propose again my original alter data hook idea.
Modules can call hook_conditional_fields_features_export_alter() and hook_conditional_fields_features_rebuild_alter() to alter the data.

vbouchet’s picture

I think our two visions are complementary. My first patch should probably be updated to provide an interface to let people decides to use or not the uuid support. So people without technical knowledge can use it without touching any line of code. For developers who want a different uuid support, we can then add your hooks so they can implement these and manage the support in a different manner.

Your patch can be integrated in conditional fields very quickly as it does not impact at all the existing feature and can't create any regression.

socialnicheguru’s picture

See comment #13 below: https://www.drupal.org/node/2373347#comment-10598774

So just to be clear for patch #4 to use uuid, I would have to explicitly set "c_fields_uuid = False" in my own code.
I also do not need the uuid module since you are using entity_get_uuid_by_id instead.

gambry’s picture

I think our two visions are complementary.
Yep, I totally agree.
Would you update patch at #4 integrating my patch and the UI switch to use UUID?

vbouchet’s picture

@SocialNicheGuru, you need to set c_fields_uuid = TRUE and enable the uuid module. The patch #4 will then alter the configuration being exported through features to export uuids insted of normal ids.

Please note that as per @gambry comment, "target_id" may not be the appropriate key and may depends on the reference field type (target_id key is provided by entity_reference module, if you use another module - such a the core term reference field, it may not work).

@gambry I think we should create another ticket to add these hooks as it is basically not specific to uuid. In addition the patch #4 is far to be ready (it only deal with "Unique value", seems to work only with entity_reference instead of being generic) and I am not sure to have time soon to work on it. Creating a separated ticket should probably increase the change to get it merged quickly (I am not the module maintainer so I can't tell when and if it will be accepted).

;-)

gambry’s picture

I think splitting will just double all the issues and the steps to have a patch approved (Proposals, Reviews, Comments, etc.), but if you won't be able to work on that bit probably creating a new ticket is not a bad idea.
I'll link the two so people can still see the progress of both and - may be - merging them back later on.

mèche’s picture

StatusFileSize
new1.84 KB

@vbouchet, I added a condition to your patch #4, to avoid errors while exporting values which are not managed by uuid (but when still using your patch somewhere else).

Status: Needs review » Needs work

The last submitted patch, 15: exporting_conditional-2373347.patch, failed testing.

mèche’s picture

StatusFileSize
new1.81 KB
vbouchet’s picture

StatusFileSize
new3.15 KB

Please find an updated patch which is more robust as checking the target entity type uses uuid, check the 'target_id' key exists (so it still does not work with fields which are not using entityreference but at least it doesnt break everything) and check a uuid is found for the target entity.
If one of these conditions does not match, the patch does not do anything and let the export in a normal state.

@Mèche, your patch can't apply properly as the path to files to patch seems specific to your environment.

mèche’s picture

StatusFileSize
new3.15 KB

@vbouchet, just a little correction (unnecessary parentheses)

mèche’s picture

StatusFileSize
new3.15 KB

Small bug on lines numbers, couldn't be applied with drush make.

shadysamir’s picture

Applied the patch from #18 but terms are still exported as tid and no mention of uuid. The vocabulary has uuid export support. Terms are exported in the same feature. What am I missing?

[EDIT] I missed the variable setting in the discussion of the issue. I had to drush vset c_fields_uuid TRUE

melonangie’s picture

StatusFileSize
new4.15 KB

Created a patch with #9 and #20, and used module_exists('uuid') instead of the variable.
[TL;DR] These patches only work with Entity Reference and NOT with Term Reference fields.

Apply the patch with patch -p1.

socialnicheguru’s picture

Status: Needs work » Needs review

in #8 there is a reference to term_reference and one of the previous patches:

- I guess you used the term_reference field and I used the entity_reference field. I should probably update the patch as this key should be dynamic.

Is there anyway to expand the patch in #22 to include term_reference.

I do like that you moved away from a variable to module_exists for uuid.

How would I test patch #22?

The last submitted patch, 18: exporting_conditional-2373347-18.patch, failed testing. View results

The last submitted patch, 20: exporting_conditional-2373347-20.patch, failed testing. View results

br0ken’s picture

Assigned: Unassigned » br0ken
Priority: Normal » Major
Issue tags: +Needs tests
StatusFileSize
new6.53 KB
br0ken’s picture

StatusFileSize
new7.35 KB
new9.88 KB

Let's have all features-related hooks together.

br0ken’s picture

Issue tags: -Needs tests
StatusFileSize
new20.23 KB
new13.15 KB

Status: Needs review » Needs work

The last submitted patch, 28: 2373347-28.patch, failed testing. View results

br0ken’s picture

Status: Needs work » Needs review
StatusFileSize
new19.95 KB
+++ b/conditional_fields.module
@@ -2023,7 +2040,7 @@ function conditional_fields_features_api() {
diff --git a/src/.DS_Store b/src/.DS_Store

diff --git a/src/.DS_Store b/src/.DS_Store
new file mode 100644

new file mode 100644
index 0000000..d2994a5

index 0000000..d2994a5
Binary files /dev/null and b/src/.DS_Store differ
diff --git a/src/Tests/.DS_Store b/src/Tests/.DS_Store

diff --git a/src/Tests/.DS_Store b/src/Tests/.DS_Store
new file mode 100644

new file mode 100644
index 0000000..5197c71

Eh...

Status: Needs review » Needs work

The last submitted patch, 30: 2373347-29.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

br0ken’s picture

StatusFileSize
new26.96 KB
new20.16 KB

The tests will be failing until someone add the test_dependencies[] to 7.x-3.x directly.

br0ken’s picture

Title: Exporting conditional fields based on a taxonomy term fails. Use uuid of term insted of tid. » Export UUIDs instead of IDs for the referenced entities
Issue summary: View changes
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 32: 2373347-32.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

br0ken’s picture

Status: Needs work » Needs review
br0ken’s picture

StatusFileSize
new26.92 KB
new369 bytes

Make it easier to apply.

Status: Needs review » Needs work

The last submitted patch, 36: 2373347-36.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

br0ken’s picture

Status: Needs work » Needs review
StatusFileSize
new27.45 KB
new1011 bytes

Prevent fatal errors when the structure of exported code was $items['entity_type:bundle:0'] = array(/* specs */);, because the new implementation expects an array of arrays (like $items['entity_type:bundle'] = array(array(/* specs */));).

Status: Needs review » Needs work

The last submitted patch, 38: 2373347-38.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

br0ken’s picture

Status: Needs work » Needs review
alfaguru’s picture

This patch also does not allow for taxonomy reference fields, which are in use in a site I just picked up. (The difference is mainly use of 'tid' instead of 'target_id'). I have hacked out a solution but rather than attach it here as the current patch does not pass testing I would suggest if anyone creates a patch that does pass they consider this case as well.

liam morland’s picture

Status: Needs review » Closed (outdated)

Drupal 7 is no longer supported. If this applies to a supported version, please re-open.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.