Drupal 6 Branch [#198672]

Comment	File	Size	Author
#48	tagadelic.info_.rej_.txt	481 bytes	WorldFallz
#48	tagadelic.info_.txt	350 bytes	WorldFallz
#47	tagadelic-20080223.patch	12.47 KB	Anonymous (not verified)
#32	tagadelic-198672-16.patch	11.28 KB	jgoldberg
#26	tagadelic-198672-15a.patch	10.75 KB	pfaocle
#25	tagadelic-198672-15.patch	11.06 KB	jgoldberg
#19	tagadelic-198672-14.patch	9.6 KB	jgoldberg
#14	tagadelic-198672-13.patch	9.36 KB	robloach
#11	tagadelic-198672-11.patch	9.35 KB	robloach
#10	tagadelic-6-x.1.patch	7.85 KB	jgoldberg
#7	tagadelic-6-x.patch	7.86 KB	jgoldberg
#6	tag_d6_update_lp_v3.patch	7.55 KB	pfaocle
#3	tagadelic_6_x_updated.patch	7.83 KB	jgoldberg
	tagadelic_6_x.patch	10.07 KB	jgoldberg

Comment #1

Bèr Kessels commented 7 December 2007 at 11:57

Status:

Needs review

» Needs work

The patch needs some cleaning:
* Don't mix tabs adn spaces. Use 2 spaces as tabs
* No witespace characters (spaces) at the end of a line please.
* The last item in an array with a comma too, please (line 72 in the patch)
* PLease add the recently added rel=tag to the links
* no spaces between functionname and ().

And why did you remove the caching?

Why do you re-introduce a "more" link? Please do not add new features in a port.

Log in or register to post comments

Comment #2

robloach

he/him

commented 7 December 2007 at 13:24

Status:

Needs work

» Closed (duplicate)

Original issue and discussion is here.

Log in or register to post comments

Comment #3

jgoldberg commented 7 December 2007 at 15:44

Status	File	Size
new	tagadelic_6_x_updated.patch	7.83 KB

Ah, oops, I diffed using the release, not HEAD. Here is the new patch.

Log in or register to post comments

Comment #4

jgoldberg commented 7 December 2007 at 20:38

Status:

Closed (duplicate)

» Needs review

Log in or register to post comments

Comment #5

pfaocle

He/Him

English

commented 11 February 2008 at 20:52

Category:	feature	» task
Status:	Needs review	» Needs work

Patch didn't apply properly for me (against HEAD from February 11, 2008 - 00:10), plus we need to add core = 6.x into the .info file to be able to install the module in Drupal 6. Having trouble accessing CVS at the moment, will try re-roll the patch and test.

Log in or register to post comments

Comment #6

pfaocle

He/Him

English

commented 11 February 2008 at 22:44

Status	File	Size
new	tag_d6_update_lp_v3.patch	7.55 KB

OK, very similar patch from me with .info change included. I'm still having issues with the new menu system (help appreciated - I've probably just missed something obvious), and we can probably take advantage of the new menu item argument wildcards for our chunk/$vid pages.

More when I get time... p

Log in or register to post comments

Comment #7

jgoldberg commented 13 February 2008 at 15:03

Status:

Needs work

» Needs review

Status	File	Size
new	tagadelic-6-x.patch	7.86 KB

It has been a while, so here is the latest state of my patch. It should be in sync with HEAD. I've been running it for quite some time with no issues. Looking forward to not being forked anymore. :)

@ leafish_paul, while you have "access arguments" set, you left out the "access callback" for your menu items. Also, you should try to use the t() function to support localization.

Log in or register to post comments

Comment #8

pfaocle

He/Him

English

commented 13 February 2008 at 17:44

@jgoldberg: AFAIK, 'access callback' defaults to 'user_access' (see here) and the t() calls are no longer required for the title or description fields (see here).

Aside from that, our patches are almost identical.

Log in or register to post comments

Comment #9

jgoldberg commented 13 February 2008 at 18:34

Ah, you're right. Not sure why your menus aren't working. Can you list your steps to reproduce?

Log in or register to post comments

Comment #10

jgoldberg commented 13 February 2008 at 18:41

Status	File	Size
new	tagadelic-6-x.1.patch	7.85 KB

FWIW...

Log in or register to post comments

Comment #11

robloach

he/him

commented 13 February 2008 at 19:27

Status	File	Size
new	tagadelic-198672-11.patch	9.35 KB

Did a couple things to it:

The block code was broken, so I fixed that
Added block level caching
Fixed warnings of undefined variables
Fixed line breaks

The hook_menu for the vocabularies should use wildcards and a wildcard loader, but we can move that to a new issue. This looks good to be moved into DRUPAL-6--1.

Log in or register to post comments

Comment #12

jgoldberg commented 13 February 2008 at 20:10

On line 358, you have:

$blocks[0]['cache'] = BLOCK_CACHE_GLOBAL;

Shouldn't it be:

$blocks[$voc->vid]['cache'] = BLOCK_CACHE_GLOBAL;

Otherwise, thanks for doing the caching correctly. We missed that.

Log in or register to post comments

Comment #13

jgoldberg commented 13 February 2008 at 20:11

Status:

Needs review

» Needs work

Log in or register to post comments

Comment #14

robloach

he/him

commented 13 February 2008 at 20:29

Status:

Needs work

» Needs review

Status	File	Size
new	tagadelic-198672-13.patch	9.36 KB

Good find.

Log in or register to post comments

Comment #15

jgoldberg commented 13 February 2008 at 20:38

Looks good to me. Tested with no problems.

Log in or register to post comments

Comment #16

robloach

he/him

commented 13 February 2008 at 20:53

Status:

Needs review

» Reviewed & tested by the community

Someone mind branching to DRUPAL-6--1 and releasing the development snapshot?

Log in or register to post comments

Comment #17

pfaocle

He/Him

English

commented 14 February 2008 at 00:05

Patch in #14 looks good, will test properly tomorrow. Minor quibbles as mentioned above, but the 'description' field for the 'admin/settings/tagadelic' menu item doesn't need a t() wrapped round it, and we can drop the 'access callback' => 'user_access', lines from the menu items. Will double check whether it's that which is causing me problems tomorrow. Nice one, all!

Log in or register to post comments

Comment #18

Bèr Kessels commented 14 February 2008 at 14:33

Status:

Reviewed & tested by the community

» Needs work

My comments. Please note that I did not dive deep into the changes for DPL6.
1 - is the init hook optimised? Is that now the preferred place to add CSS? I prefer to have it placed only when required: when building the block contents and/or when building the pages.
2 - Is taxonomy_get_vocabularies not deprecated? used in tagadelic_menu()
3 - IMO drupal 6 has default ways to set the title of a block, can we not (ab)use that, instead of implementing our own? see L 189 etc

Good catch: the $delta = 0 instead of O... :)

Thanks a million so far!

Log in or register to post comments

Comment #19

jgoldberg commented 14 February 2008 at 16:06

Status:

Needs work

» Needs review

Status	File	Size
new	tagadelic-198672-14.patch	9.6 KB

@ Ber Kessels

1. Using hook_init is the best practice for adding CSS; if you look at some of the other core modules, you'll see they do the same thing. Now, I believe CSS is always imported regardless of what page you are looking at, which is the same behavior as adding the css in an if (!$may_cache) block. In my opinion, you gain a marginal improvement of performance for an increased amount of complexity, so it is probably better to make a separate patch that people can use if they need that kind of extreme performance requirements.

2. The function didn't change, just the label. See http://drupal.org/node/114774#taxonomy-load

3. Yes, we should do that instead.

@leafish_paul

Like you said, it doesn't make a difference whether or not we have an access callback set. However, we don't really gain anything by not having it either, except maybe a piece of code that is 4 lines shorter. One argument for keeping it in: it might make it easier on a novice user trying to debug if the code is clear about which access callback is getting called. BTW, did you get a chance to try your own code with the callbacks to see if it improved anything?

Attached is a re-rolled patch that includes removing our title form element from the block configuration and the localization function for the description field. It also adds some commenting for hook_init.

Log in or register to post comments

Comment #20

robloach

he/him

commented 14 February 2008 at 17:51

Priority:	Normal	» Critical
Status:	Needs review	» Needs work

Three issues. One major, one nitpick, one minor:

With this patch, tagadelic.css will be added on every page you visit, even if there isn't any tagadelic being shown. It would be better if it is included only when it is needed. Including it in hook_init is not the best practice. The best practice is to include CSS only when it has to be included.
Don't need 'access callback' => 'user_access' in the hook_menu, but doesn't negatively effect it
The call to taxonomy_get_vocabularies in hook_menu should be handled with %wildcards

Log in or register to post comments

Comment #21

pfaocle

He/Him

English

commented 14 February 2008 at 17:56

Priority:

Critical

» Normal

Cheers for the update. "we don't really gain anything by not having it either, except maybe a piece of code that is 4 lines shorter": suppose not, that's fine with me.

Patch applies fine, seems to working bar a couple of problems. I:

Used a clean install of Drupal 6
Patched tagadelic HEAD with patch #19, installed
Used devel generate module to create 3 vocabs
Edited these vocabs to ensure they're set to 'Story' node type and 'Tags' on
Enabled the three corresponding tagadelic blocks, and 'tags in this post' block for good luck
Generated 20 or so terms, then created 50 nodes placed into these terms.

The problems were firstly, only two out of three paths to /tagadelic/chunk/vid worked (for vids 1 and 3, but not 2). I tried again with new terms/content, to find all of the paths working correctly. Perhaps we really need to get wildcards in for these paths? Or perhaps this was symptomatic of this problem, ie not only are we showing an empty block when there are no posts in the relevant vocabulary, but the more link (/tagadelic/chunk/vid) throws a 404.

Secondly, I enabled the tagadelic blocks for the three vocabularies before adding any terms. The blocks remained empty after adding 20 or so terms and it was only when I manually emptied the cache via devel that the terms appeared. Bar the menu/path problem mentioned above, the corresponding /tagadelic/chunk/vid pages worked OK and showed the correct terms. Also, if I removed all terms, then added 3 more (empty) vocabs, the blocks remained with the old terms listed. Again, emptying the cache remedied this.

Adding a new tag to an article doesn't immediately add the tag to the block for the same reason, I guess: something to do with the block cache settings introduced in #11? Incidentally, all the Drupal core cache/performance settings were set to their defaults (off bar gzip).

A tentative 'needs work' then, if anyone else notices anything iffy with caching or paths? Run out of time today, hope this helps.

Log in or register to post comments

Comment #22

pfaocle

He/Him

English

commented 14 February 2008 at 18:01

Priority:

Normal

» Critical

Un-hijacking-Rob's priority change.

Log in or register to post comments

Comment #23

robloach

he/him

commented 14 February 2008 at 20:09

Having the block only update when the cache is cleared is reasonable (this is what I added in #11. This is managed via the block-level caching system, and the cron job which is already part of Drupal. If we have it clear the cache everytime a node is updated or changed, it could potentially really slow down the system. If you read more on block caching, you'll see that we have the best values selected for the job.

This leaves us with the menu problem (implement the wildcard system for the menu), and having the CSS load only when tagadelic is displayed.

Log in or register to post comments

Comment #24

jgoldberg commented 15 February 2008 at 03:41

Correct me if I'm wrong, but it seems like loading the CSS only when the blocks display isn't trivial. If caching is enabled, there isn't really a clean way to determine if a block got displayed or not since hook_block($op = 'view') stops getting called. I'm sure there is a way to abuse the core to figure it out, but we shouldn't go down that path.

We could propose adding an extra op to hook_block additional to 'view', which allows operations to be run even if the block is cached. Not sure if we want to go down that path.

Of course, the simplest solution would be to always just include the stylesheet. :)

Anyone care to add some clarity I might be missing?

Log in or register to post comments

Comment #25

jgoldberg commented 15 February 2008 at 05:16

Status:

Needs work

» Needs review

Status	File	Size
new	tagadelic-198672-15.patch	11.06 KB

Attached is a re-rolled patch that includes wildcard handling. Originally, both tagadelic_page_chunk and tagadelic_page_handler parsed the vocs argument themselves:

function tagadelic_page_chunk() {
  $vocs = arg(2);

  if (is_numeric($vocs)) {
    $vocs = array($vocs);
  }
  elseif (preg_match('/^([0-9]+,){1,5}[0-9]+$/', $vocs)) {
    $vocs = explode(',', $vocs);
  }
  elseif($vocs == NULL) { //create a chunk from all free tagging vocs
   foreach (taxonomy_get_vocabularies(NULL) as $vocabulary) {
      $vocs[] = $vocabulary->vid;
    }
  }
...

Here is the new wildcard handler:

function tagadelic_vocs_load($vocs) {
  if (is_numeric($vocs)) {
    $vocs = array($vocs);
  }
  elseif (preg_match('/^([0-9]+,){1,5}[0-9]+$/', $vocs)) {
    $vocs = explode(',', $vocs);
  }
  return $vocs;
}

Since both tagadelic_page_chunk and tagadelic_page_list share this function, I leave out the NULL edge case and let it get handled in its respective function:

function tagadelic_page_chunk($vocs) {

  if ($vocs == NULL) {
  	foreach (taxonomy_get_vocabularies(NULL) as $vocabulary) {
      $vocs[] = $vocabulary->vid;
    }
  }
...

function tagadelic_page_list($vocs) {

  if ($vocs == NULL) {
  	return drupal_not_found();
  }
...

I'm going to mark this patch as ready for review, but veto if you still think there is a way to include the stylesheet only when the blocks display.

Log in or register to post comments

Comment #26

pfaocle

He/Him

English

commented 15 February 2008 at 16:12

Status	File	Size
new	tagadelic-198672-15a.patch	10.75 KB

Looking good, applies, works. Attached is a very minor revision (tabs -> spaces).

So:

* CSS loading problems - shall we move to another issue?
* Block caching options seem reasonable, behaves as expected AFAI can see.
* Wildcards/callbacks seem to be working.

Probably OT here, but after playing around with this module a bit more, I've noticed some inconsistencies in the menu items, for example: a page not found is shown for /tagadelic/chunk/1 when there are no nodes to display, but with /tagadelic/list/1 we see a page with the vocabulary title and description... anyways, that's probably better here.

Should get time to test some a D5->D6 update with tagadelic over the weekend.

Log in or register to post comments

Comment #27

jgoldberg commented 15 February 2008 at 16:59

Probably OT here, but after playing around with this module a bit more, I've noticed some inconsistencies in the menu items, for example: a page not found is shown for /tagadelic/chunk/1 when there are no nodes to display, but with /tagadelic/list/1 we see a page with the vocabulary title and description... anyways, that's probably better here.

I just tested that in Drupal 5 and the same result occurs.

This patch shouldn't be focused on upgrading to 6 AND solving every defect out there. We should just get a first release out before we start worrying when the stylesheet is included, what happens if list/5 has no nodes, etc.

Log in or register to post comments

Comment #28

Bèr Kessels commented 17 February 2008 at 20:33

jgoldberg and leafish are correct: We should not try to solve bugs here, just upgrade to 6. Debugging is done after that.
so here is my list:
1- leave the CSS loading where it was.
2- leave any 404s readmores and so forth the way they were, even if you consider them broken.

Lets try and make this patch as small and focused as possible.

Log in or register to post comments

Comment #29

jgoldberg commented 17 February 2008 at 20:59

@ Bèr

The #26 patch meets those two requirements. So, as long as no one is running into any regressions, we should be good to go. Can someone else out there take it for a spin before we mark it RTBC?

Log in or register to post comments

Comment #30

add1sun commented 17 February 2008 at 23:43

I just did a quick run through on this on my newly upgraded site.

Used tagadelic HEAD and applied the patch in #26. I enabled the "Tags in tags" vocab block and worked fine. I then went in and configured the block to show up to 50 tags. When I saved that change my block title (the default one it had) disappeared. So something about setting the default block title isn't set quite right. I don't have time to poke around right now, just wanted to give a heads up on a minor issue.

Other than that it seems to be working fine in my cursory review.

Log in or register to post comments

Comment #31

pfaocle

He/Him

English

commented 18 February 2008 at 00:05

Status:

Needs review

» Needs work

Can easily reproduce add1sun's problem. Shouldn't we be inserting the default 'Tags in vocab' title into the block configuration form (under $op == 'configure')? Otherwise our 'tagadelic_block_title_'. $delta variable will be empty, the default won't get used and our block titles will also be empty.

Log in or register to post comments

Comment #32

jgoldberg commented 18 February 2008 at 01:18

Status:

Needs work

» Needs review

Status	File	Size
new	tagadelic-198672-16.patch	11.28 KB

Re-rolled a new patch that fixes the issue brought up in #30. Thanks for finding that add1sun.

Log in or register to post comments

Comment #33

Bèr Kessels commented 18 February 2008 at 12:00

patch in #32 looks alright, from the outside. I do, however, want some testresults before putting it out in the wild. Can someone run this on an existing environment (with tags) and on a clean one?

Log in or register to post comments

Comment #34

limitedability commented 19 February 2008 at 02:41

Can someone please post a patched archive? I'm having trouble patching it myself. It's patching but giving errors on several lines. I've tried it on the latest official build and some past builds, and each one is giving me errors.

Thanks.

Log in or register to post comments

Comment #35

add1sun commented 19 February 2008 at 03:51

@limitedability, you need to patch against HEAD, not the latest release.

Log in or register to post comments

Comment #36

pfaocle

He/Him

English

commented 19 February 2008 at 13:49

#32 has fixed the block title issue here, nice one. Running this on clean dev/test Drupal 6 site at the moment without problems (so far). Haven't had chance to try a 'real' update yet.

Log in or register to post comments

Comment #37

robloach

he/him

commented 19 February 2008 at 15:56

Status:

Needs review

» Reviewed & tested by the community

The issue with the custom CSS code having to be in hook_init() will have to wait until we have custom CSS and JS files for cached blocks. Until then, this is the only solution available.

I've quickly gone through #32 and it seems good. I think we should commit this, and then branch it to DRUPAL-6--1.

Log in or register to post comments

Comment #38

jgoldberg commented 19 February 2008 at 23:13

Status:

Reviewed & tested by the community

» Needs review

Bèr would like one more positive test result before putting out a stable 6-x release. Anyone else who could help would be highly appreciated.

Log in or register to post comments

Comment #39

DenRaf commented 20 February 2008 at 16:05

I can confirm a positive test. However I would suggest to make one little change.
After applying patch: tagadelic-198672-16.patch from post #32 I suggest this:

In tagadelic.module change line 231:
from: cache_set($cache_name, $tags);
to: cache_set($cache_name, $tags, ‘cache_page’, CACHE_TEMPORARY);

This way are the change you make in your content and tags, way faster updated in your tagcloud.

Log in or register to post comments

Comment #40

jgoldberg commented 20 February 2008 at 17:42

Status:

Needs review

» Reviewed & tested by the community

@ DenRaf

While I think there is a good case for using CACHE_TEMPORARY rather than the default value CACHE_PERMANENT (which requires calling cache_clear_all with the explicit cache name for tagadelic in your cron.php), at this point we are only trying to reproduce the 5-x behavior, which uses the CACHE_PERMANENT constant. We should open a ticket for this question though (perhaps give the user the option in the admin panel).

Thanks for reporting a positive test result.

Log in or register to post comments

Comment #41

zilla commented 20 February 2008 at 23:36

hello, sorry to sound like such a nube here but i'd really like to try tagadelic in a new d6 install but do not know how to apply patches to the module directly...is there a link to a most currently patched version or a simple explanation of how to download the most recent patch (in 32 above) and apply to the download full mod from the mod directory ?

Log in or register to post comments

Comment #42

jgoldberg commented 21 February 2008 at 00:25

Check back within the next few days, a release should be coming soon.

Log in or register to post comments

Comment #43

zilla commented 21 February 2008 at 15:54

thanks! happy to test the release and report back and bugs or conflicts with other modules etc...

Log in or register to post comments

Comment #44

Bèr Kessels commented 21 February 2008 at 21:13

I'd like one more -or so- positive test before I will release a stable. If none come in befor etomorrow, I'l try to release a beta before the weekend.

Log in or register to post comments

Comment #45

stefan vaduva commented 22 February 2008 at 09:17

I've also tested it with Drupal 6 and I don't have any problems.

Log in or register to post comments

Comment #46

Anonymous (not verified) commented 24 February 2008 at 01:47

+1 on this, works great.

Log in or register to post comments

Comment #47

Anonymous (not verified) commented 24 February 2008 at 02:11

Status:

Reviewed & tested by the community

» Needs review

Status	File	Size
new	tagadelic-20080223.patch	12.47 KB

Cleaned up the code a bit to comply with coding standards.

Log in or register to post comments

Comment #48

WorldFallz commented 24 February 2008 at 07:37

Status	File	Size
new	tagadelic.info_.txt	350 bytes
new	tagadelic.info_.rej_.txt	481 bytes

Just tried this patch-- failed on hunk 1 (the .info file) against the current HEAD. I've attached the reject file in case that can be of an use. I fixed the .info but I've no idea how to patch a patch, lol. It's also attached in case that can be of any assistance. Thanks for the d6 port of this most critical module!

Log in or register to post comments

Comment #49

add1sun commented 24 February 2008 at 15:23

Hm, well the patch in #47 applies just fine for me against HEAD checked out from CVS. Your file looks like it has the packaging script additions in it, i.e. that it is from a tarball and not the CVS repo, so I am assuming that is why you get a fail since the patch assumes a "clean" info file from CVS and does not take into account the stuff that d.o adds while packaging.

So, basically, I'm saying that the patch is fine.

Log in or register to post comments

Comment #50

WorldFallz commented 24 February 2008 at 16:56

ah... I wasn't aware it made a difference. Thanks for the info. From now on I'll get the files to patch from cvs.

Log in or register to post comments

Comment #51

jgoldberg commented 24 February 2008 at 17:10

Status:

Needs review

» Reviewed & tested by the community

I didn't have any problems with the patch in #47. I looked at the changes and all of them are just spacing adjustments. We should be good to go without any new testing (although, of course, more testing is perfectly fine.)

Log in or register to post comments

Comment #52

zilla commented 26 February 2008 at 23:13

hello. just reading this last comment (51) and wondering if the patch has been rolled into a new or current drupal 6 dev version or beta version...any status update appreciated.

Log in or register to post comments

Comment #53

robloach

he/him

commented 27 February 2008 at 15:42

Title:	Drupal 6-x Patch	» Drupal 6 Branch
Assigned:	Unassigned	» robloach
Status:	Reviewed & tested by the community	» Fixed

http://drupal.org/cvs?commit=103421

I've committed the patch to HEAD and then moved it to DRUPAL-6--1. Any more issues and patches regarding the Drupal 6 port will have to be made from DRUPAL-6--1. I've create the 6.x-1.x-dev development snapshot release, as well as a 6.x-1.0 release of Tagadelic. Thanks, everyone, for the help on this!

Log in or register to post comments

Comment #54

zilla commented 27 February 2008 at 16:09

thanks! just downloaded and putting up within seconds...

Log in or register to post comments

Comment #55

Anonymous (not verified) commented 12 March 2008 at 16:11

Status:

Fixed

» Closed (fixed)

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

Log in or register to post comments

Comment #56

princedudley commented 29 March 2010 at 08:08

Category:	task	» support
Status:	Closed (fixed)	» Active

Can't anybody organize all the patches and fixes into an orderly manner and then post it?
Its very confusing to see one here and another one somewhere else.
Without proper explanition and instructions, the patches are as good as nothing.
There should be versioning even for the patches!!!

Log in or register to post comments

Comment #57

WorldFallz commented 29 March 2010 at 13:44

Status:

Active

» Closed (fixed)

@princedudley did you not read the entire thread? See #53.

Log in or register to post comments

Drupal 6 Branch

Comments