Merge duplicate terms form improvements

Haven't looked too deep just a quick drive by review.

+++ b/term_merge.pages.inc
@@ -86,16 +86,16 @@ function term_merge_form($form, $form_state, $vocabulary = NULL, $term = NULL) {
-      '#title' => t('Terms to Merge'),
-      '#description' => t('Please, choose the terms you want to merge into another term.'),
-      '#ajax' => array(
-        'callback' => 'term_merge_form_term_trunk',
-        'wrapper' => 'term-merge-form-term-trunk',
-        'method' => 'replace',
-        'effect' => 'fade',
-      ),
-      '#default_value' => $term_branch_value,
-    ) + $form['term_branch'];
+        '#title' => t('Terms to Merge'),
+        '#description' => t('Please, choose the terms you want to merge into another term.'),
+        '#ajax' => array(
+          'callback' => 'term_merge_form_term_trunk',
+          'wrapper' => 'term-merge-form-term-trunk',
+          'method' => 'replace',
+          'effect' => 'fade',
+        ),
+        '#default_value' => $term_branch_value,
+      ) + $form['term_branch'];

This indent doesn't look necessary and muddies up the patch. It's 2 spaces already.

Hello!

I've looked into MrDaleSmith patch and extended it a bit further. I really liked the idea to include ability to introduce fields, attached to terms, onto the resultset of potential duplicates.

However, I am not sure the ability to search only for a single term duplicates is something that makes sense to the general audience of this module - why would someone ever want to merge only duplicates of term X but not the duplicates of the terms Y and Z? If they are all duplicates, then likely the end user will want to merge all of them. So at the moment I do not want to include this feature into the module. Maybe if more people speak up and request this feature, then yes, but otherwise I would hold it off.

Therefore my patch includes the code changes related to adding fields into the table of possible duplicates. Reviews and feedback are welcome!

Patch #6 tested at simpletest.me

I have added few terms and duplicates and when i visit:

admin/structure/taxonomy/tags/merge/duplicates

This error appear:

Fatal error: Unsupported operand types in /home/df558/www/sites/default/modules/term_merge/term_merge.pages.inc on line 717

And after that, if I click browser "back" I get a dozen of these:

Notice: Undefined index: fields in term_merge_duplicates_form() (line 692 of /home/df558/www/sites/default/modules/term_merge/term_merge.pages.inc).
Warning: Invalid argument supplied for foreach() in term_merge_duplicates_form() (line 692 of /home/df558/www/sites/default/modules/term_merge/term_merge.pages.inc).
Notice: Undefined index: fields in term_merge_duplicates_form() (line 692 of /home/df558/www/sites/default/modules/term_merge/term_merge.pages.inc).
Warning: Invalid argument supplied for foreach() in term_merge_duplicates_form() (line 692 of /home/df558/www/sites/default/modules/term_merge/term_merge.pages.inc).
... ...

I've sent you a link to simpletest.me project with this bug visible.

Tested #8. It works.

Nice per-term basis merge options with "Select all" checkbox for those who need to merge all duplicates (previous default behavior).

I like it.

Good work bucefal91, MrDaleSmith!

Why would someone ever want to merge only duplicates of term X but not the duplicates of the terms Y and Z?

Homonyms (homographs) are such example. If two or more homonyms exist in vocabulary they should be excluded from merging during bulk merge process because they have different meanings, so they should be kept as separate terms although they share the same spelling.

English homonyms: https://en.wikipedia.org/wiki/List_of_true_homonyms
English homographs: https://en.wikipedia.org/wiki/List_of_English_homographs

English language is not so rich with homonyms, but there are languages where homonyms are very common. Having ability to exclude them from merging on per-term basis during bulk merge is very useful.

Hello!

I am enclosing another patch, where I tried to leverage Fields module to render the values of term fields. Basically we can trying convert field values into HTML using some tricks inside of Term Merge module, but I do not like this approach, because in theory there can be unlimited amount of field types and we won't be able to "play nice" with each of them. Instead, we can outsource this task to the Field module, who actually must "play nice" with all field types. That's the idea behind this patch.

It will display field values in the same way as those fields would appear in their default formatter. I think it's quite optimal.

Also, I fixed a minor XSS vulnerability - the field label wasn't escaped before being inserted into the table header, so if a hacker created a field with XSS in the field title, then this JS would get executed.

Hi.

Tested patch #11

It will display field values in the same way as those fields would appear in their default formatter. I think it's quite optimal.

It works as expected. I have tested it with booleans, numbers, text fields, term reference fields, image field, file field, entityreference field (contrib), date field (contrib), address fileld (contrib). Default display works nice for both single and multiple fields for all these field types.

Image field works as expected as well, but it displays Original image as default value which is defenitely not optimal display for our usecase. Thumbnail (100x100) would be better. Floating left thumbnails would be nice as well in case of multiple-valued image field.

Marking as "reviewed and tested" since all filed displays work as expected... however, if you want to do some more custom work on image field display here before committing you can change status to "needs work" until done.

P.S. I have changed the title of this issue to reflect more precise what this issue is about. Removed "Minor" from title as well. It's not so minor any more. :)

I resisted my laziness as hard as I could... So here's the same patch as above + the images are displayed in thumbnail size. Though I didn't make it float left (that's where my laziness had a frightening defeating over my will).

Anyway, I think it's more than good so we can commit it soon :)

Tested #13. It works.

For multi-value image fields it is probably enough to list first image only (delta 0). This would make merge form more compact.

This is just a tool to distinguish between terms who share the same name, so listing all images of multi-value image fields is probably not necessary.

I've committed the last patch. I think it's okay to show all images and after all, term merge is an admin module, so maybe it doesn't hurt to show a bit more content at the price of making the page a little bit too noisy. I'd rather throw it out there in the public and see if users start to file feature requests.

Thank you, everybody, for your work :)