I've create a patch to improve Dart Taxonomy. When I initially converted the module I didn't pay full attention to the hook_dart_key_vals function. I finally got a change to improve on it so that taxonomy terms are fetched from all term reference fields on a node.

I've also made a small change to the noscript tag building in dart.module as it was repeating sz and pos and the ord/tile didn't work.

CommentFileSizeAuthor
#7 dart-tax.patch2.95 KBbleen
#2 dart_taxonomy_improved_and_noscript_fixed-1249646-3.patch4.08 KBjamiecuthill
#1 dart_taxonomy_improved_and_noscript_fixed-1249646-2.patch4.09 KBjamiecuthill
dart_taxonomy_improved_and_noscript_fixed--1.patch3.88 KBjamiecuthill

Comments

jamiecuthill’s picture

jamiecuthill commented
StatusFileSize
new dart_taxonomy_improved_and_noscript_fixed-1249646-2.patch4.09 KB

Please ignore file #1 as I forgot one line change for the noscript tag.

I will update this issue if there are any other fixes that I need to make.

jamiecuthill’s picture

jamiecuthill commented
StatusFileSize
new dart_taxonomy_improved_and_noscript_fixed-1249646-3.patch4.08 KB

Another change, I'm having a little trouble with the tile ordering and numbers with the tags set by context but I think it's a external issue to this module.

bleen’s picture

bleen commented

jamiecuthill: the patch in #2 looks pretty good (thanks!) but I need to take a bit more time to look at it more closely. In general though, please make sure you keep separate issues separate (ex. one issue for the taxonomy improvements & one for the proposed change to the noscript tag) . It just makes reviewing and committing so much easier.

I'll give this some time later today or tomorrow

THANKS!

jamiecuthill’s picture

jamiecuthill commented

Thanks for getting back so quick and sorry for the bundling up the issues - I'll separate them out next time. If need be I can create a separate issue for noscript and roll a new patch for both.

Just to describe the issue with noscript.

1. sz and pos were duplicated as they were being hard coded into the $src variable and then also coming from $tag->key_vals.
2. The $special_key_vals array doesn't appear to be indexed by key so the isset() calls for tile and ord were not working.
3. The tile values were being incremented before they were added into the tag so it was skipping the dart_special_tile_init value.

bleen’s picture

bleen commented
Status: Needs review » Needs work 
+++ b/dart.module
@@ -284,32 +284,35 @@ function dart_tag_prepare_noscript($tag) {
+    elseif ($key_val['key'] == 'ord') {
+      $ord = $ord == 0 ? rand(1000000000, 9999999999) . '?' : $ord;

I dont think this should be an "else". What if I want both a tile AND an ord? 

+++ b/modules/dart_taxonomy/dart_taxonomy.module
@@ -210,13 +210,29 @@ function dart_taxonomy_dart_key_vals($tag) {
+    // If this is a node, grab the taxonomy terms from all taxonomy fields.

"taxonomy fields" should be "term reference fields"

jamiecuthill’s picture

jamiecuthill commented

The else if there because it's contained in a foreach, the key will never be tile and ord at the same time. tile and ord will come on different iterations on the loop.

Agreed with the taxonomy term field, sloppy commenting by me there.

bleen’s picture

bleen commented
Status: Needs work » Needs review
StatusFileSize
new dart-tax.patch2.95 KB

Ok ... I have played with this patch pretty thoroughly and everything looks pretty good except the changes to "tile". I have tested several times and the current handling of tile seems to work fine for me. jamiecuthill, could you do me afavor and open an seperate issue explaining exactly the behavior you are seeing in more detail.

In the mean time I have made some minor changes to the other part of patch you submitted, but fundementally it looks good. Please give this a quick test and I'm prepared to commit it.

jamiecuthill’s picture

jamiecuthill commented

That patch is working perfectly. Thanks.

I'll revisit the changes to dart.module in a new issue.

bleen’s picture

bleen commented
Status: Needs review » Reviewed & tested by the community

Ill take that as a RTBC

bleen’s picture

bleen commented

Committed to 7.x-2.x

bleen’s picture

bleen commented
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.