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]

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.

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.

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?

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:
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 )
);
- 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:
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. :D

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:
echo "The content of my field is $array['key'] now.";Will not evaluate good.

So you will have to use:
echo "The content of my field is $array[key] now.";
or
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.

[Mr Blunt]

Ahhh, so the periods basically join your 3 "seperate strings" in the last example, one of which doesn't use double quotes since it's a var .... and those joining periods never get put into the actual saved string.

It's sinking in I think.

So if I declare an array key with $var['keyname'], and I come across a place where I'd rather call it with a simple $var[keyname], will it fly in most situations or must I do something stupid like $var[keyname] = $var['keyname'] beforehand? .... in which case it would probably be best to just use a new keyname to distinguish the difference?

I guess the reason I'm getting so technical is that I'm afraid of using non-arrayed variables as I don't want to conflict with anyone elses vars .... sooo one of my goals, as you might notice from my php file, was to try and handle all my vars from mainly one single array named "whodl" (3 really, but who's counting LOL).

Part of me says I'm going too far since it's a self contained webpage being displayed one time, and since control isn't going back to the origin page, I should be able to declare just about any simple (non-array) var that I want .... but then the inexperience in me thinks that's sloppy code so I've taken the long route so far.

[Marco van Herwaarden]

So if I declare an array key with $var['keyname'], and I come across a place where I'd rather call it with a simple $var[keyname], will it fly in most situations or must I do something stupid like $var[keyname] = $var['keyname'] beforehand? .... in which case it would probably be best to just use a new keyname to distinguish the difference?
You don't need to declare the version without the quotes. It will fail however if you have defined a constant with the same name as your keyname.

I even in a seperate script prefix most of my var's (since you are also including standard vB scripts, there could still be a name conflict even in a standalone script).

[Mr Blunt]

Just a follow-up....

Thank you BIGTIME for the help here.
I finished a beta copy of my script.
I'll leave the old code here, but it shouldn't be used by anyone.
Maybe someone else will learn from comparing this vs. that.


https://vborg.vbsupport.ru/showthread.php?t=93167


I don't know if anyone is up for it, but I'd sure love to have someone double check my work. I feel pretty confident, but between a ton of rewrites and a wireless keyboard that sometimes skips a character LOL ... another set of eyes to check me would be awesome because I DO care about the people who install my products.

And if anyone does check it out .... I'm up for any suggestions, ideas, and or little coding tips if you feel like sharing them with me.

[Mr Blunt]

Somewhat important follow-up.
The code examples above don't reflect UNSIGNED integer values.
vBulletin uses unsigned for these 4 INT values, so I should too.

$db->query("CREATE TABLE " . TABLE_PREFIX . "blunts_whodownloaded_ip (
blunts_whodownloaded_ipid INT(10) UNSIGNED NOT NULL PRIMARY KEY AUTO_INCREMENT,
userid INT(10) UNSIGNED NOT NULL default 0,
username VARCHAR(100) NOT NULL default '',
filename VARCHAR(100) NOT NULL default '',
fileid INT(10) UNSIGNED NOT NULL default 0,
ipaddress VARCHAR(15) NOT NULL default '',
alt_ip VARCHAR(15) NOT NULL default '',
dateline INT(10) UNSIGNED NOT NULL default 0
)");

Here's a quote from dev.mysql.com
http://dev.mysql.com/doc/mysql/en/numeric-types.html
All integer types can have an optional (non-standard) attribute UNSIGNED. Unsigned values can be used when you want to allow only non-negative numbers in a column and you need a bigger upper numeric range for the column.

As of MySQL 4.0.2, floating-point and fixed-point types also can be UNSIGNED. As with integer types, this attribute prevents negative values from being stored in the column. However, unlike the integer types, the upper range of column values remains the same.

And from the same thread:
For example, the range of an INT column is -2147483648 to 2147483647. If you try to insert -9999999999 into an INT column, MySQL clips the value to the lower endpoint of the range and stores -2147483648 instead. Similarly, if you try to insert 9999999999, MySQL clips the value to the upper endpoint of the range and stores 2147483647 instead.

If the INT column is UNSIGNED, the size of the column's range is the same but its endpoints shift up to 0 and 4294967295. If you try to store -9999999999 and 9999999999, the values stored in the column are 0 and 4294967296.

When a floating-point or fixed-point column is assigned a value that exceeds the range implied by the specified (or default) precision and scale, MySQL stores the value representing the corresponding end point of that range.


In a nutshell .... Not only does unsigned not allow a negative number to be inserted, but an unsigned column allows for DOUBLE the amount of possible positive integer values than a signed column .... unless floating point numbers are involved.

Just trying to help the next guy....
:D

[Marco van Herwaarden]

Yes that is the reason some fields are defined as unsigned.