Coding advise about adding to a db column?

[Mr Blunt]

I seek advise from anyone willing to give it.

I've been supporting a hack to track who downloaded each file for a long time and I recently ported it to vb350 no sweat. The hack simply writes a comma & the attachmentid# to a new db text column called "DOWNLOADS" in the table "USER".


I want to advance the hack to the next level.
I want to now add the dateline, ip, and alt_ip with each downloaded file.
But I don't know if any other HACKS use this column.


I'm done with all the code and it works peachy.
I only use the same old DOWNLOADS column.
It appends a colon inbetween each new datastring.


So here's an old sample from the database:

4,20,18

Here's a new sample (ip's fudged obviously):

4:1121263806:88.88.88.88:88.88.88.88,20:1121263837:88.88.88.88:88.88.88.88,18:1121263946:88.88.88.88:88.88.88.88

(edited to say vbulletin is adding that space, not me, meaning my db entry/read doesn't have spaces so act like the string is whole please)


So my question is three-fold
1) Is it normal for a coder to make a huge db change like this?
2) Does the idea of me doing this piss anyone off?
3) Are there any other hacks that use a column named as mine which I obviously wouldn't want to conflict with?


The last thing I want to do is make anyone mad so I'm asking before I release this revamped version.

I will say that I made it so it can still read the "old data type".
So what "was once tracked" will still be "readable".

But obviously this doesn't mean other hacks would understand my "new data type".

[Dream]

little suggestion, you could use a "whodownloaded_" prefix in each field, so youll know no one else will use those fields

[Marco van Herwaarden]

As i understand you want to put all this in a single field. I would suggest against it, and would advice you to create a new table for this with seperate fields. If you want to keep it in a single field in the user table i suggest you change your data into an array and srialize it into the field.

It is always a good idea to prefix you fields (and tables) with something, so you know you won't get any conflicts.

[Mr Blunt]

Originally Posted by Dream

little suggestion, you could use a "whodownloaded_" prefix in each field, so youll know no one else will use those fields

No suggestion is little to me.
Please, by all means speak your mind guys.
I'm still learning and WANT to learn.

First off, it's "just one" field.
Called "downloads" under the "user" table.

It's not a vbulletin field.
It's a custom field added to vbulletin's "user" table.
Downside is TWTCommish used a very common name for his field.

I was trying to avoid changing to a new field as I was hoping to keep this compatible with the old whodownloaded hack which many have been using for years.

Originally Posted by MarcoH64

As i understand you want to put all this in a single field. I would suggest against it, and would advice you to create a new table for this with seperate fields.

Yes, and I had a feeling you would say this.
I just had a nice crash course in handling arrays so I guess I could pull this off if it's really neccessary.

Originally Posted by MarcoH64

If you want to keep it in a single field in the user table i suggest you change your data into an array and srialize it into the field.

I haven't learned about serializing yet. Is this neccessary because of things like special characters in the text I'm writing to the database or for some other reason?

Originally Posted by MarcoH64

It is always a good idea to prefix you fields (and tables) with something, so you know you won't get any conflicts.

Duely noted and I will for hacks I write in future.
I was just working with what was around before me.



Hey, would it help if I showed my php code?
Then you can at least see what I'm doing before and after the database.
The code and variable names are pretty easy to follow.
I'll gladly post if someone's maybe willing to help my quest.

Here's my "attachment_complete" hook that writes to the db

$colon = ':';
$ip = IPADDRESS;
$alt_ip = ALT_IP;
$dateline = TIMENOW;
$userid = $vbulletin->userinfo['userid'];
$attachmentid = $vbulletin->input->clean_gpc('r', 'attachmentid', TYPE_UINT);

if ($userid AND $attachmentid AND $dateline)
{
	$dl = $db->query_first("
		SELECT downloads
		FROM " . TABLE_PREFIX . "user
		WHERE userid = $userid
	");
	$comma = ($dl[downloads]) ? ',' : '';
	if ($vbulletin->options['logip'])
	{
		$db->query_write("
			UPDATE " . TABLE_PREFIX . "user
			SET downloads = '$dl[downloads]$comma$attachmentid$colon$dateline$colon$ip$colon$alt_ip'
			WHERE userid = $userid
		");
	}
	else
	{
		$db->query_write("
			UPDATE " . TABLE_PREFIX . "user
			SET downloads = '$dl[downloads]$comma$attachmentid$colon$dateline$colon$colon'
			WHERE userid = $userid
		");
	}
};

Here's my extension php file to handle the reading and array handling.

<?php

require_once("./global.php");
$attachmentid = $vbulletin->input->clean_gpc('r', 'attachmentid', TYPE_UINT);

$whodl = array();
$whodl[shownames] = false;
$whodl[showdates] = false;
$whodl[showips] = false;
$whodl[showaltips] = false;

if ($show['whodladmin'])
{
	$whodl[shownames] = true;
	$whodl[showdates] = true;
	$whodl[showips] = true;
	$whodl[showaltips] = true;
}
else
{
	if ($show['whodlnames'])
	{
		$whodl[shownames] = true;
	}
}

if ($whodl[shownames])
{
	$whodl[rows] = 0;
	$whodl[tracked] = 0;
	
	if ($attachmentid)
	{
		$poster = $db->query_first("
			SELECT userid, dateline, filename, visible, counter, postid
			FROM " . TABLE_PREFIX . "attachment
			WHERE attachmentid = $attachmentid
		");

		if ($poster[visible] = 1)
		{
			if (!$poster[userid] OR $poster[userid] == 0)
			{
				$poster[userid] = 0;
				$poster[username] = $vbphrase['unregistered'];
			}
			else
			{
				$postername = $db->query_first("
					SELECT username
					FROM " . TABLE_PREFIX . "user
					WHERE userid = $poster[userid]
				");
			}

			if ($show['whodldatestoposter'] AND $vbulletin->userinfo['userid'] == $poster[userid])
			{
				$whodl[showdates] = true;
			}

			$whodl[postername] = $postername[username];
			$whodl[posterid] = $poster[userid];
			$whodl[posterdate] = vbdate($vbulletin->options['dateformat'],$poster[dateline]);
			$whodl[filename] = $poster[filename];
			$whodl[counter] = $poster[counter];
			$whodl[postid] = $poster[postid];

			$whodlrows = $db->query_read("
				SELECT userid, username, downloads
				FROM " . TABLE_PREFIX . "user
				WHERE downloads LIKE \"$attachmentid:%\" OR downloads LIKE \"%,$attachmentid:%\" OR downloads LIKE \"$attachmentid,%\" OR downloads LIKE \"%,$attachmentid,%\" OR downloads LIKE \"%,$attachmentid\"
				ORDER BY username ASC
			");

			if ($db->num_rows($whodlrows))
			{
				$whodl[rows] = $db->num_rows($whodlrows);
				while ($whodlrow = $db->fetch_array($whodlrows))
				{
					$whodl[usersid] = $whodlrow[userid];
					$whodl[usersname] = $whodlrow[username];
					$whodluser = preg_replace('/:/', ',', $whodlrow[downloads], -1);
					$whodluser = preg_split('/,/', $whodluser, -1);
					$whodluser = array_count_values($whodluser);
					$whodl[userscount] = $whodluser[$attachmentid];
					$whodl[tracked] += $whodl[userscount];
					eval('$whodownloadsbit .= "' . fetch_template('whodownloadsbit') . '";');

					if ($whodl[showdates] OR $whodl[showips] OR $whodl[showaltips])
					{
						$whodlextras = preg_split('/,/', $whodlrow[downloads], -1);
						foreach ($whodlextras as $whodlextra)
						{
							$whodlextra = preg_split('/:/', $whodlextra, -1);
							if ($whodlextra[0] == $attachmentid)
							{
								$whodl[date] = $whodl[showdates] ? vbdate($vbulletin->options['dateformat'],$whodlextra[1]) : '';
								$whodl[ip] = $whodl[showips] ? $whodlextra[2] : '';
								$whodl[alt_ip] = $whodl[showaltips] ? $whodlextra[3] : '';
								eval('$whodownloadsbit .= "' . fetch_template('whodownloadsdate') . '";');
							}
						}
					}
				}
			}
			else
			{
				$whodl[error] = 'No Users Found';
				eval('$whodownloadsbit .= "' . fetch_template('whodownloadsbit') . '";');
			}
		}
		else
		{
			$whodl[error] = 'File Not Found';
			eval('$whodownloadsbit .= "' . fetch_template('whodownloadsbit') . '";');
		}
	}
	else
	{
		$whodl[error] = 'File Not Found';
		eval('$whodownloadsbit .= "' . fetch_template('whodownloadsbit') . '";');
	}
}
else
{
	$whodl[error] = 'Feature Disabled Or No Permission';
	eval('$whodownloadsbit .= "' . fetch_template('whodownloadsbit') . '";');
}
eval('print_output("' . fetch_template('whodownloads') . '");');

?>

and then I add two new $show variables with a global hook to handle permissions for the ability to view the button which opens my whodownloaded popup.

$show['whodldatestoposter'] = true;
$show['whodladmin'] = iif($vbulletin->userinfo['usergroupid'] == 6, true, false);
$show['whodlnames'] = iif($vbulletin->userinfo['usergroupid'] == 5 OR $vbulletin->userinfo['usergroupid'] == 7 OR $vbulletin->userinfo['usergroupid'] == 2, true, false);

[Mr Blunt]

I added 3 posts back on july 16th and they automerged so might need to look up and refresh yourself if you read the post early.


Can someone tell me if I'm on the right track??
If want to add a brand new table with 8 fields via admincp?
Is this the query I (and my users) would run?
(check my syntax please)

CREATE TABLE `whodownloaded` (
	`downloadid` INT(15) NOT NULL AUTO_INCREMENT,
	`userid` INT(10) NOT NULL default '0',
	`username` VARCHAR(100) NOT NULL default '',
	`fileid` INT(10) NOT NULL default '0',
	`filename` VARCHAR(100) NOT NULL default '',
	`dateline` INT(10) NOT NULL default '',
	`ipaddress` VARCHAR(15) NOT NULL default '',
	`alt_ip` VARCHAR(15) NOT NULL default '',
PRIMARY KEY ( `downloadid` ) 
) TYPE = MYISAM;

I'm sure that isn't right as I never write anything right the first time.
I care about the users who are going to install this so I'd like it to be precise if someone can help me.

And then how does this look for my plugin to populate the tables as users download attachments??

<plugins>
	<plugin active="1">
		<title>Who Downloaded - Enable Logging</title>
		<hookname>attachment_complete</hookname>
		<phpcode><![CDATA[$whodl = array();
$whodl[userid] = $vbulletin->userinfo['userid'] ? $vbulletin->userinfo['userid'] : 0;
$whodl[username] = $vbulletin->userinfo['username'] ? $vbulletin->userinfo['username'] : '';
$whodl[fileid] = $vbulletin->input->clean_gpc('r', 'attachmentid', TYPE_UINT);
$whodl[filename] = $vbulletin->input->clean_gpc('r', 'filename', TYPE_STR);
$whodl[dateline] = TIMENOW;
$whodl[ipaddress] = $vbulletin->options['logip'] ? IPADDRESS : '';
$whodl[alt_ip] = $vbulletin->options['logip'] ? ALT_IP : '';

if ($whodl[userid] AND $whodl[fileid] AND $whodl[dateline])
{
	$db->query_write("
		INSERT INTO " . TABLE_PREFIX . "whodownloaded
			(userid,
			username,
			fileid,
			filename,
			dateline,
			ipaddress,
			alt_ip)
		VALUES
			($whodl[userid],
			$whodl[username],
			$whodl[fileid],
			$whodl[filename],
			$whodl[dateline],
			$whodl[ipaddress],
			$whodl[alt_ip])
	");
};]]></phpcode>
	</plugin>
</plugins>

You can see the variables I'm wanting to save.
Is this an effective way to do it?
And even if it's an "effective way" ... is there a "better way"??

No suggestion or idea is too little in my eyes.

There's screenshots of "my vision" posted way down at the end of my whodownloaded thread (in 350 extensions forum) if anyone is wondering why I'm trying to handle so many variables in that big php file I posted in earlier post.

[Marco van Herwaarden]

The query is correct, but i would suggest some minor changes:
[sql]CREATE TABLE whodownloaded (
downloadid INT(15) NOT NULL AUTO_INCREMENT,
userid INT(10) NOT NULL default 0,
username VARCHAR(100) NOT NULL default '',
fileid INT(10) NOT NULL default 0,
filename` VARCHAR(100) NOT NULL default '',
dateline INT(10) NOT NULL default '',
ipaddress VARCHAR(15) NOT NULL default '',
alt_ip VARCHAR(15) NOT NULL default '',
PRIMARY KEY (downloadid )
);[/sql]
- Removed the backticks, they are not needed.
- Removed the single quotes around the default values for numeric columns.
- Removed the table type MyIsam, not needed, can follow the database default.

[Mr Blunt]

Thank You MarcoH64!!

Question 101
Now that I'm using my own table, are there any restrictions for field names, and in fact the short the better, correct?? Only point is to maybe make them friendly enough so coders understand the code, right? Like for example 'ip' would be better than 'ipaddress' ... providing of course that I pass all my arrays & variables correctly ..... OR ..... do coders making their own tables typically try to use the exact same field names as vbulletin so coders don't have to track down and/or think of how the variable got passed?? Another good example that I'd like to change is 'fileid' instead of the long 'attachmentid' .... OR ..... Are you guys saying that I really should change ALL field names to be unique names like real short 'dl', 'uid', 'uname', 'fid', 'fname', date, ip, altip


Question 102
So about the prefix...
You and Dream said:

Originally Posted by MarcoH64

It is always a good idea to prefix you fields (and tables) with something, so you know you won't get any conflicts.

Are you guys saying should I do something like split that INTO a "prefix.name" like "who.downloaded"?

And if so ... and I understand this right ... this would be in addition to the prefix declared in a user's config ... meaning my db calls would be like:

FROM " . TABLE_PREFIX . "who.downloaded


And if all that is true .... is "who" technically a bad choice for a prefix meaning I should make it something extremely arbitrary such as:

FROM " . TABLE_PREFIX . "blunts.whodownloaded

[Marco van Herwaarden]

101:
You are completly free in this, since this is your own table. It is a style issue, but if a column has an identical meaning as a standard vB column (userid for example) i would prefer to use the standard name. If they have different content, they should best be named different. It is always a strugle between short and meaningfull, it helps if code can easily be read by others, and names are kinda self-explanatory.

I always end up with names that are too long, so i need to type a bit more.

Also good practise (but again no law, although some vB admin functions asume this by default) is to name the key column (mostly an auto increment column) after the tablename followed by 'id', so whodownloadedid in your example.

102:
No need to prefix fields in your own table, only when adding to existing tables. Don't use a period (.) between prefix and name. In your case a good prefix might be bwd_ (Blunts WhoDownloaded), but you are again free in this. Just use something that is not very likely to cause conflicts, now or in future.

The '...." . TABLE_PREFIX . "tablename...' should be used in every SQL-statement. It will get replaced by the vB table prefix as defined in the config.php.

[Mr Blunt]

Very clear!!
I follow you!!
Thanks VERY much for taking the time with me!!

I'm trying to hold my questions to the minimum.
I'll make my table and play around with it and my code.
I think I can handle it from here.

Hopefully you and/or anyone else will help evaluate a final draft in a few days.
And especially since IP's are very sensative data by nature.
I'd really appreciate a double checker before I release.
I'm still a grasshopper and I'm not afraid to admit it!!!


Ohhhh ... one parting question:

Question 103
How come sometimes people use [keyname] while others use ['keyname'] when getting and declaring values in an array (and I don't neccessarily mean in the queries, I mean while handling arrays themselves too)?? Syntax always seems to be my biggest pitfall so any tips in this department??

[Marco van Herwaarden]

It is always the best to use the $array['keyname'] syntax, but sometimes that don't evaluate so easy, for example if you use it inside double quotes:

PHP Code:

echo "The content of my field is $array['key'] now."; 


Will not evaluate good.

So you will have to use:

PHP Code:

echo "The content of my field is $array[key] now."; 


or

PHP Code:

echo "The content of my field is " . $array['key'] . " now."; 


where the last is the best, but most (including me often) are too lazy for that.