search

SylvainTI / phpliteadmin

Automatically exported from code.google.com/p/phpliteadmin
0 stars 0 forks source link

Remove view=1 parameter #193

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
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'

Original issue reported on code.google.com by crazy4ch...@gmail.com on 17 Mar 2013 at 11:16

GoogleCodeExporter commented 9 years ago 
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.

Original comment by dreadnaut on 4 Jan 2014 at 5:18

Attachments:

GoogleCodeExporter commented 9 years ago 
Looks good! :)

Original comment by crazy4ch...@gmail.com on 4 Jan 2014 at 5:36

GoogleCodeExporter commented 9 years ago 
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.

Original comment by dreadnaut on 4 Jan 2014 at 6:04

Attachments:

GoogleCodeExporter commented 9 years ago 
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!

Original comment by dreadnaut on 4 Jan 2014 at 6:13

Attachments:

GoogleCodeExporter commented 9 years ago 
Whops, I found some more view=1 around the code. Here's the complete patch.

Original comment by dreadnaut on 4 Jan 2014 at 6:17

Attachments:

GoogleCodeExporter commented 9 years ago 
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.

Original comment by crazy4ch...@gmail.com on 4 Jan 2014 at 6:22

GoogleCodeExporter commented 9 years ago 
Updated to include quote_id()

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

Original comment by dreadnaut on 4 Jan 2014 at 6:27

Attachments:

GoogleCodeExporter commented 9 years ago 
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.

Original comment by crazy4ch...@gmail.com on 4 Jan 2014 at 6:34

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

Original comment by dreadnaut on 4 Jan 2014 at 6:37

Attachments:

GoogleCodeExporter commented 9 years ago 
Ah sorry. Looks good. Go for it! (commit)

Original comment by crazy4ch...@gmail.com on 4 Jan 2014 at 6:38

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision r450.

Original comment by dreadnaut on 4 Jan 2014 at 6:56

GoogleCodeExporter commented 9 years ago 
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)

Original comment by crazy4ch...@gmail.com on 4 Jan 2014 at 7:18