Remove view=1 parameter

[Christopher Kramer]

Originally reported on Google Code with ID 193

Currently, we have a GET parameter "view=1" that is present if the current "table" is
not a table but actually a view.

Having this parameter has some disadvantages in my opinion:
- we might forget to pass it at some points. This leads to bugs like issue #191.
- we have a parameter called "view" that is used for the different db-tabs and has
completely different semantics.
- there is a lot of redundant code around this that could be removed together with
the parameter

Instead of having this parameter, I would propose to detect whether the "current table"
is a view or a table. This is as simple as this:
SELECT type FROM sqlite_master WHERE name='name of current table'

Reported by crazy4chrissi on 2013-03-17 23:16:47

[dreadnaut]

I started working on this, and got caught up in cleaning more $_GET excess :)

First step: collect the value $_GET['table'] once, early in the script, reducing the
number of isset() checks and making it possible to check if the table is actually a
view. $target_table is now used to hold the value.

Reported by dreadnaut on 2014-01-04 17:18:30

<hr> * Attachment: target_table.diff

[Christopher Kramer]

Looks good! :)

Reported by crazy4chrissi on 2014-01-04 17:36:01

[dreadnaut]

Step 2: add Database->getTypeOfTable($table) which returns 'view' or 'table', and replace
all $_GET['view'] checks relative to the table/view setting.

Turns out, there was just *one* place where the 'view' parameter was used for other
reasons.

Reported by dreadnaut on 2014-01-04 18:04:01

<hr> * Attachment: no_get_view.diff

[dreadnaut]

Lastly, some clean up: in some places a $table variable was actually filled with $_GET['table'],
so I replaced that with our new $target_table.

I'd say I'm done with this, ready for review and commit!

Reported by dreadnaut on 2014-01-04 18:13:55 - Status changed: Started

<hr> * Attachment: no_get_view_clean.diff

[dreadnaut]

Whops, I found some more view=1 around the code. Here's the complete patch.

Reported by dreadnaut on 2014-01-04 18:17:47

<hr> * Attachment: no_get_view_clean_clean.diff

[Christopher Kramer]

Thanks. Looks good. And amazing how many places we add view=1 just to for one simple
place where we check for it and checking is so easy ;)

Only one thing: Please use $db->quote_id($table):
$result = $this->select("SELECT * FROM `sqlite_master` WHERE `name`=".$db->quote_id($table),
'assoc');

$table could contain quotes or stuff...

Next step would be to make sure the is no view=1 left.

Reported by crazy4chrissi on 2014-01-04 18:22:32

[dreadnaut]

Updated to include quote_id()

$result = $this->select("SELECT `type` FROM `sqlite_master` WHERE `name`='"
 . $this->quote_id($table) . "'", 'assoc');

Reported by dreadnaut on 2014-01-04 18:27:43

<hr> * Attachment: no_get_view_clean_clean_quote.diff

[Christopher Kramer]

You don't need quotes around quote_id() as it adds them automatically.
And maybe better use quote in this case, as from an SQL point of view, this is a value
that should be in single quotes and not a tablename that should be in double quotes.
Although SQLite does not care much.
$result = $this->select("SELECT `type` FROM `sqlite_master` WHERE `name`="
 . $this->quote($table), 'assoc');

And I think getTypeOfTable should be part of the Database class.

Reported by crazy4chrissi on 2014-01-04 18:34:10

[dreadnaut]

Here the version with quote(). getTypeOfTable is already in the Database class, I'm
using $this if you check.

Reported by dreadnaut on 2014-01-04 18:37:14

<hr> * Attachment: no_get_view_clean_clean_quote2.diff

[Christopher Kramer]

Ah sorry. Looks good. Go for it! (commit)

Reported by crazy4chrissi on 2014-01-04 18:38:32

[dreadnaut]

This issue was closed by revision r450.

Reported by dreadnaut on 2014-01-04 18:56:51 - Status changed: Fixed

[Christopher Kramer]

Thanks for fixing this.

As this affects only maintainability and some more testing might be good, I think we
don't need to push it into upcoming 1.9.5 final. So it will make it into 1.9.6.

(I'd like to have 1.9.5 RC1 and 1.9.5 to be exactly the same code if no bugs found
in RC1)

Reported by crazy4chrissi on 2014-01-04 19:18:52 - Labels added: Target-1.9.6