Module: List ordering

All files from this thread

File Name	File Size		Posted by	Date
list_ordering.zip	6.6 KB	open | download	sdbbs	09/12/2009	Read message

List Ordering v.0.0.1

Adds a &ordering= query string parameter, allowing for different sorting of threads in list view - and implements this as clickable table headers in list.tpl. Requires hacks ...

Installation: copy the contents of this folder as phorum5/mods/list_ordering (only info.txt, list_ordering.php and settings.php are necessary), apply any hacks (see .diff file) and then enable from admin.php.

For more, see included readme.

Good that someone took this on.
hooks in the database layer are a no-go for us but if it works for your hack ... .
Also you should note that the user should have keys on the fields which are sorted. didn't check yet which additional keys could be needed.
Why are you using $_REQUEST instead of the regular $PHORUM['args'] as used for the internal vars?

---

Thomas Seifert

Hi Thomas,

Thanks for your response ! :)

Quote

hooks in the database layer are a no-go for us

Sorry about that - I could have imagined that is the case, but I had no other idea how to solve that ordering business without intervening in the database layer. Was it maybe better to go for a full hack, rather than trying to separate stuff as modules?

Quote

Also you should note that the user should have keys on the fields

Thanks about this, wasn't aware of it - I guess I should look through the developer doc a bit more about this..

Quote

Why are you using $_REQUEST instead of the regular $PHORUM['args']

Honestly - because I wasn't really aware they were there :) Just what worked easiest for the moment.. I guess I *really* should look through the developer doc a bit more :)

Thanks again for the response - and all suggestions are welcome :)
Cheers!

Some remarks on the module code:

I guess you should remove this one from the diff: "hack for import phorum3 to phorume5 duplicate user import: ignore keys here"

Same question as Thomas: why fiddle with $_REQUEST? You are actively changing $_REQUEST at the start, which might be a very bad idea. Phorum could be running in some portable/embedded environment where URLs are created differently. If such environment adds extra params to the query string after yours, then you are now effectively clipping them off the URL. Thomas' suggesion is a good one: always go with $PHORUM['args'] (and in its company with phorum_get_url() to build the URL with your parameter in it).

The following piece of code is in your module:

	if (isset($_REQUEST["ordering"])) {
		switch ($_REQUEST["ordering"]) {
			case "subject":
				$sortorder = $_REQUEST["ordering"] . " ASC";
				break;
			case "modifystamp":
				$sortorder = $_REQUEST["ordering"] . " DESC";
				break;
			default: // keep DESC as in original mysql.php
				$sortorder = $_REQUEST["ordering"] . " DESC";
				break;
		}
		
	}

You've shown one of the very good reasons to not include hooks in de database layer code here. We do not want modules to meddle with the SQL code, because it can lead to insecure code. The red part is very bad practice, since it allows for arbitrary SQL injection via the URL. Please, fix this issue a.s.a.p. If you do not want to keep a full registry of valid options (like you already started with "subject" and "modifystamp", then at least do some sanity checking on the input. For example:

Language: PHP
default:
    if (preg_match(';/^\w+\s+(?:ASC|DESC)$/i';, $_REQUEST[';ordering';])) {
        $sortorder = $_REQUEST[';ordering';];
    } elseif (preg_match(';/^\w+$/';, $_REQUEST[';ordering';])) {
        $sortorder = $_REQUEST[';ordering';] . '; DESC';; // keep DESC as in original mysql.php
    } else {
        trigger_error("Illegal ordering parameter", E_USER_ERROR);
    }
    break;

---

Maurice Makaay
Phorum Development Team
my blog linkedin profile secret sauce

Quote
sdbbs
Sorry about that - I could have imagined that is the case, but I had no other idea how to solve that ordering business without intervening in the database layer. Was it maybe better to go for a full hack, rather than trying to separate stuff as modules?

A full hack is even worse IMO. But that's because I am such a big fan of modules. If I would code this, I would write database query code in my module to do the required querying, instead of modifying the original db layer code.

Quote
sdbbs

Quote

Also you should note that the user should have keys on the fields

Thanks about this, wasn't aware of it - I guess I should look through the developer doc a bit more about this..

You won't find anything about this in the developer docs. This is related to standard SQL query optimization. So if you want to learn about that, then mysql.com would be a better starting point.

---

Maurice Makaay
Phorum Development Team
my blog linkedin profile secret sauce

Hi,

Maurice, thanks for the response !!

Quote
Maurice
Same question as Thomas: why fiddle with $_REQUEST? You are actively changing $_REQUEST at the start, which might be a very bad idea. Phorum could be running in some portable/embedded environment where URLs are created differently. If such environment adds extra params to the query string after yours, then you are now effectively clipping them off the URL. Thomas' suggesion is a good one: always go with $PHORUM['args'] (and in its company with phorum_get_url() to build the URL with your parameter in it).

Thanks for the clarification - before this, I wasn't aware of any portable possibilities :) And thanks on the pointer on what should be implemented instead.

Quote
Maurice
You've shown one of the very good reasons to not include hooks in de database layer code here.

Thanks for pointing that out - actually, I totally didn't think about that, SQL injections are the last thing on my mind when I'm just supposed to "make something work"; only after some days after reading Thomas' response, I guessed that this (SQL injection) may be the case.

In the meantime, I just posted a new module (Module: Move message(s)), that does exactly this:

Quote
Maurice
I would write database query code in my module to do the required querying, instead of modifying the original db layer code.

And also, that new module, at a certain point, calls a default Phorum5 function which queries the database and returns an array; and then I re-sort the array in php. It's a performance hit, possibly - but it should be much safer than injecting SQL in the database.

In any case - thanks for the pointers guys; I will try to implement them as a new version of the module, but I cannot tell how soon that will be...

Cheers !

Hi! Just curious, have you ever had the chance to implement the changes Maurice had suggested? Thanks!