Tagadelic blocks should link to full tags page (read more link)

After so long, this issue is still valid. This is nothing more then a bump :)

I have this working for the 4.7x-2.0 tar. I don't have a site running the cvs head at the minute - so the patch is against that tar file - sorry.

It allows the user to choose whether to turn the more link on or not, and the user can choose cloud or list.

No idea if its a usable patch - just some changes I hacked in - but working for me on http://www.chrissearle.org/

I prefer to not have this in a setting, but instead in a proper theme function.

At the very least, the HTML for the read more link must be themable. Outputting P tags and the likes without a theme function is not an option.

Was wondering about that myself. But - I haven't figured out programming modules for theme stuff yet - I just hacked this together to get what I needed :)

I'm still looking thru the APIs to see what is available for drupal (up to now I've just used the theme/modules as available on drupal.org - not done any coding) - so - at some point soon I should hopefully be able to update the patch.

OK. Looks like the module uses theme_tagadelic_weighted function. Are you suggesting modifying this - or would you prefer a separate function? And am I right in thinking that by doing it as a theme function then the only way to choose on/off is by editing a theme?

A new theme function please. '_weighted' is used for building weighted clouds, not for outputting readmore stuff.

And yes, the only way would then be to change a theme. This is no different in Drupal core and 90% of the modules. Its the Drupal way of doing things.

Ok - that's cool. I'll try and take a look tonight. Still learning :)

Ok - is this any better? Still against the 4.7x-2.0

Tried this out tonight and works great apart from:

/tagadelic/list/$vid

needs to be:

tagadelic/list/$vid

on my site it would not add the base path with that forward slash there. I would also say change 'all tags' to 'more tags' as I believe it is limited to the number set in the options (not 100% sure about that though).

Apart from that nice work, I found it very useful, and can also verify it works fine on v5 although I did patch by hand as it was simple.

Good point about the leading / - removed.

all -> more - tested. Yes - it is limited by the setting in admin - so changing the text of the link.

New patch attached.

Next question. I only have the 4.x-2.0 tagadelic installed. I'd like to provide both 4.x and 5.x patches for this (given that people think the patch is OK) - I guess I should grab the DRUPAL-4 and DRUPAL-5 cvs checkouts for that? Should be against CVS rather than a nightly for example?

OK - think I figured out a bit more on cvs tags. New patch - this is against the CVS checkout for the DRUPAL-4-7 tag.

Committed to 4.7 branch. It will be included in the next 4.7 release. I will wait a little more to see if there are other things to do on the 4.7 branch, else I will release a nw version with only this feature added in 4.7.

OK - here would be the diff against the DRUPAL-5 cvs branch

I've decided to link to chunk/$id instead of list/id, the chunk is he default view.

Fair enough. As far as I can see you could choose chunk, list or cloud - I preferred list - since that was what _I_ wanted on my site. If chunk is default elsewhere then chunk it is :)

I already committed it to the 5.x branch. Was worknig on it when you committed the patch.

No worries. First time I've sent in a patch - is it normal to send on one for 4 and one for 5 - or would it have been better against HEAD?

I've added a conditional to only display if there are more tags than the amount set in the block settings:

<?php
      $total_terms = db_num_rows(db_query("SELECT tid FROM "."{term_data} WHERE vid = '" . $delta ."'"));
      if($total_terms > variable_get('tagadelic_block_tags_'. $delta, 12)){
		$blocks['content'] .= theme('tagadelic_more', $voc->vid);//add more link
      }
?>

means an extra db call though, what do you think?

You are entirely right!

The extra DB call should not be a SELECT * though, but rather a SELECT COUNT(*). And if you really care about performance, add a LIMIT:

$one_more = variable_get('tagadelic_block_tags_'. $delta, 12) + 1;
$count = db_result(db_query(SELECT COUNT(tid) FROM {term_data} WHERE vid = %d LIMIT %d, $delta, $one_more)):
if ($one_more > $count) { //...
}

A query with LIMIT and COUNT is very performant even on large datasets.

NOTE: NEVER ever concanate stuff into a SQL like you did with $delta. It opens a security hole.

Very interesting, thanks for the info I didn't know about the sql count. I also cannot believe I didn't think about the limit to +1 :)

Could you tell me what the %d is for? Also if possible could you give me a hint to why " . $delta ." is different and dangerous? I have used this method for years, you have me slightly worried.

Thanks for you help and knowledge. There were a couple of small errors in your code above(took a few minutes to spot the colon at the end!), the full code is now:

<?php>
	$one_more = variable_get('tagadelic_block_tags_'. $delta, 12) + 1;
	$count = db_result(db_query('SELECT COUNT(tid) FROM {term_data} WHERE vid = %d LIMIT %d, $delta, $one_more'));
	if ($one_more > $count) {
		$blocks['content'] .= theme('tagadelic_more', $voc->vid);//add more link
	}
?>

Thanks again, tagedelic/ free tagging vocs have been the most enjoyable aspect of drupal 5 so far for me.

The security issues with dumping strings into SQL just like that, are calledSQL injection.

I suggest you read http://drupal.org/node/62304 and the great introduction at wikipedia: http://en.wikipedia.org/wiki/SQL_injection

Hmm.

Just applied this (from #20). Now - where there are more tags available the more tags link is missing, where all the tags are already shown, the more tags link is present.

Also there is an issue with the position of the closing ' on the db_query line in #20 is there not?

I would think that

db_query('SELECT COUNT(tid) FROM {term_data} WHERE vid = %d LIMIT %d, $delta, $one_more')

should be

db_query('SELECT COUNT(tid) FROM {term_data} WHERE vid = %d LIMIT %d', $delta, $one_more)

Second question - if $delta is the vid - should the line

$blocks['content'] .= theme('tagadelic_more', $voc->vid);

be

$blocks['content'] .= theme('tagadelic_more', $delta);

yes, both your questions are answered positive: my code was merely quick scetchy pseudo-code. Hence I did not even bother to change the status.

Hi,

I just installed this 5.x-1.x-dex version and I noticed the "more tags" link appears although there are no more tags to display.

I looked at your code here, and believe :

    if ($one_more > $count) {

Should it be ? :

    if ($one_more == $count) {

The reason being if $one_more is indeed > $count, than we have enough ($count) tags displayed, and only when $count is equal to $one_more (i.e. the tag limit+1), we have excess tags to display ?

I've lost track of the status on this one. I now have 5.x-1.0-rc1 installed. Is everything in place apart from the conditional (comments 18 and on)? Is that what needs sorting and forming as a patch now? I can have a go at that if that is what is wanted :)

OK - attached a patch against CVS DRUPAL-5 - seems to work for me :) Based on the discussion and comments above. Answer to comment #24 - I think it needs to be <= not ==. == would then only trigger when the number of tags was exactly the count + 1, not under AND not over.

@chrissearle, You say, you wanted <= Less then or equal to. However, you implemented less then (and not equal to).
Though I prefer the latter (less then), it makes me wonder why you wrote different code from your comment in #24.

Bèr

Very good question. Wish I knew :) I think that's actually a bug in the patch :( it's to do with when you have one more tag than number shown in a block.

Let's work it thru :)

$trigger_count = variable_get('tagadelic_block_tags_'. $delta, 12) + 1;

trigger_count is the "max number of tags in block" + 1 - so - lets take the default example

Number of tags to show in block 12

trigger_count 13

So - if you have 1 - 12 tags then you don't want to show the link

13 tags and up you do want the link.

So - if you use < then

12 tags gives 13 < 12 - no link - correct

13 tags gives 13 < 13 - no link - wrong

14 tags gives 13 < 14 - link - correct.

So - it seems to me that <= is correct?

If you agree - fixed patch attached - one of them should be right :)

Any further action needed on the patch here?

Any progress on this ?

I earlier used the "==" conditional because of the LIMIT (to $one_more) applied in the SQL query.

Has this been committed yet or does it need more testing?

I am running the version in post #28 http://drupal.org/files/issues/tagadelic.module_1.patch which is behaving exactly as I expect it to.

My fault thatthis took so long.

But since 6 is the Drefault and preferred Drupal version, I think it is a bad idea to implement new features on our 5 branch. We should best focus ths on 6.

Well - I've also updated to 6 now (qv: http://drupal.org/node/305380) - so that's fine by me ;)

Looking at the latest CVS of this. I was getting the more tags link when I had either more tags to show (correct) OR exactly the number of tags available as shown (incorrect).

    if ($voc = taxonomy_vocabulary_load($delta)) {
      $blocks['subject'] = variable_get('tagadelic_block_title_'. $delta, t('Tags in @voc', array('@voc' => $voc->name)));
      $tags = tagadelic_get_weighted_tags(array($voc->vid), variable_get('tagadelic_levels', 6), variable_get('tagadelic_block_tags_'. $delta, 12));
      $tags = tagadelic_sort_tags($tags);
      $blocks['content'] = theme('tagadelic_weighted', $tags);//return a chunk of 12 tags
      if (count($tags) >= variable_get('tagadelic_block_tags_'. $delta, 12)) {
        $blocks['content'] .= theme('tagadelic_more', $voc->vid);//add more link
      }
    }

I believe that to fix this

      if (count($tags) >= variable_get('tagadelic_block_tags_'. $delta, 12)) {

should be

      if (count($tags) > variable_get('tagadelic_block_tags_'. $delta, 12)) {

But - testing is showing that this is doing something that was a little unexpected.

I have three terms - and so I tested a block with n as my number of tags to show.

#tags to show    #count tags     #link shown      #link wanted
1                1               yes              yes
2                2               yes              yes
3                3               yes              no
4                3               no               no
5                3               no               no

Since the return of count(tags) is based on a list that is already restricted by the #tags to show setting (see the call to tagadelic_get_weighted_tags) then the comparison here is flawed. count(tags) can NEVER be larger than the max number for the block.

Note - if you replace the conditional with just > rather than >= then the link is never shown. This is again due to that count(tags) is based on the restricted list.

The greater than check I believe strongly to be correct given that count(tags) is the number of tags in the taxonomy (total) - but not when count(tags) is the number of tags (already restricted).

I'll try to get some time to think about this one here over xmas.

Attached patch fixes this issue. But - it introduces a second use of cache and may therefore be more complex than is wanted. Needs some review ;)

the cache keys should be changed to become like the cache keys in tagadelic_get_weighted_tags().

Closing "needs work" that has been open for a long time, without anyone working on it.