Unify view/action handling for database and table pages

[dreadnaut]

Originally reported on Google Code with ID 155

Currently we have two GET parameters choosing which content to show. Database tabs are
chosen with view=, while table tabs are selected with action=. *Both* are also used
for other effect!

This is not only confusing, but adds to the general convolution of the code. I propose
to unify them, possibly under a new page= parameter, as a first step to separate the
output code from the rest.

The first patch (against r305, attached below) renames "actions" and "views" so they
follow a common style: [database|table]_<name of the tab shown>.

Reported by dreadnaut on 2012-11-22 19:20:18

<hr> * Attachment: prefixes.diff

[Christopher Kramer]

Hmm. I am not sure. Another proposition:

Two parameters. One specifying the object (table/database/row/column), one specifying
the action (structure/browse/drop/insert/edit/...).

e.g.
object=table&action=browse
object=database&action=structure
object=row&action=delete&rowid=123

Problem with your approach is: If you want to check what object we want to change (without
checking what action we want to perform), we need to check whether there is a table_*
or a database_*, which is not as simple as it should be.

Reported by crazy4chrissi on 2012-11-22 23:29:06

[dreadnaut]

I am not sure I understand what you mean by "If you want to check what object we want
to change".

These are only "views", pages that do not perform any write operation on the database.
They only show information: a list of rows, a list of column, and text form, a confirmation
form, etc.

When we talk about "actions" on the database, then it's a different matter.

Maybe the title is a bit confusing, but so is the code :D   I don't want to unify all
actions, but only the view= and action= parameter as they are use to select pages,
see diff above.

Reported by dreadnaut on 2012-11-23 00:18:50

[Christopher Kramer]

It's not correct that stuff triggered by "action=" does not write anything to the db
(at the moment). If there is a "confirm=1" parameter as well, actions will actually
be performed.
And I really think this makes sense.

What would you do instead of
   if(isset($_GET['action'])) // it's about a table, not a db!
With your approach?
   if(isset($_GET['page']) && strpos($_GET['page'],'table_')===0) ?
This does not look like an improvement to me.


And what would you do with these stuff that actually performs actions:
phpliteadmin.php?table=test&action=row_delete&confirm=1&pk=12

This actually deletes row number 12 in table test.

Reported by crazy4chrissi on 2012-11-23 09:50:50

[dreadnaut]

Some stuff triggered by action writes to the db, some does not. Some behaves exactly
like view= pages, but goes through a completely different branch of code.

And we get lines like 4592

if(!isset($_GET['table']) && !isset($_GET['confirm']) && (!isset($_GET['action']) ||
(isset($_GET['action']) && $_GET['action']!="table_create"))) //the absence of these
fields means we are viewing the database homepage

If we split acting code and non acting code, we can start dividing like this:

if (isset($_GET['action']))
  switch ($action) {
    ... do something, produce result, choose page ...
  }

switch ($page) {
... show pages ...

  default:
     <show home>
}

Something that is not possible when "pages" are not distinguishable from "actions",
if not by checking other parameters case-by-case.

Reported by dreadnaut on 2012-11-23 10:46:00

[Christopher Kramer]

Hmm.. I am not saying the current code is a good approach.
But why not keep separation between acting and non-acting code using the "confirm"
parameter?

> Some stuff triggered by action writes to the db, some does not.
    It should only to so if confirm is present.
> Some behaves exactly like view= pages, but goes through a completely different branch
of code.
    Placing code badly is another story (and truely an issue).

We could get rid all all this "do we have anything? if not, we are on the home page"
stuff (line 4592) by simply doing an
if(!isset($_GET['action'])) $action='structure';
if(!isset($_GET['object'])) $object='database';

We'd need something like this anyway because this won't work if $page is not set:
switch ($page) {
... show pages ...

  default:
     <show home>
}
> Something that is not possible when "pages" are not distinguishable from "actions",
if not by checking other parameters case-by-case.
Not case-by-case. 1 additional parameter to distinguish "pages" from "action", namely
"confirm":

if(!isset($_GET['confirm']))
{
switch ($action) {
... show pages ...

  default:
     <show home>
}
}

Reported by crazy4chrissi on 2012-11-23 11:12:24

[Christopher Kramer]

Actually, my point is not really whether we distinguish acting code with confirm or
using action/page. It does not make much difference.

My point is that with page=table_browse we put 2 things into one parameter which is
a bad idea. My point is to have 2 parameters (e.g. action/object, page/object - i don't
care here).

Reported by crazy4chrissi on 2012-11-23 11:20:51

[dreadnaut]

Having two parameters would have sense if we had a set of objects (e.g. database, table),
and a set of operations that can be applied to any object. We don't; what we have:

- the current database, stored as session data (therefore not a parameter *)
- a set of operations that take parameters, among with there might be a table

For example, as you cannot run 'row_insert' on a database, there is no need of an 'object'
parameter to say "please notice, we are working on a table now".

Two parameters only make for a more complicated set of ifs.

> We'd need something like this anyway because this won't work if $page is not set:
> switch ($page) {
> ... show pages ...
> 
>  default:
>     <show home>
> }

Add this before the switch ;-)

$page = $page ?: 'home';


[*] it would be nice if it were, but let's leave that for a separate issue

Reported by dreadnaut on 2012-11-23 11:52:52

[Christopher Kramer]

Okay. I don't wanna go on forever like this because my main interest regarding this
project is not how we call parameters or if we use 2 or 3 of them.

So here are my hopefully last arguments about this topic:
__

> Having two parameters would have sense if we had a set of objects (e.g. database,
table), and a set of operations that can be applied to any object.

Usually, objects are defined by the operations that can be applied on them. Therefore,
there usually are not 2 types of objects on which exactly the same operations can be
applied on - otherwise it would quite likely to be one type (maybe a subtype).
But what usually is the case is that some operations can be performed on a number of
different objects. Table and Database for example share at least these operations:
create, drop, rename, view_structure, import, export, vacuum, perform sql
I'd say this is quite a lot.

I mean in SQL the operations are not called DATABASE_CREATE, TABLE_CREATE and DATABASE_DROP
and TABLE_DROP (as would be following your logic) but are called [operation] [object]
(CREATE DATABASE, CREATE TABLE, DROP DATABASE, ...).  For exactly the same reason.
(And it is like this although not all operations can be performed on all objects, e.g.
COMMIT is an action that can only be applied on TRANSACTION. In SQLite a lot of other
combinations are missing like ALTER DATABASE (which is available in lots of other DBMS).)


> For example, as you cannot run 'row_insert' on a database, there is no need of an
'object' parameter to say "please notice, we are working on a table now".

"row_insert" is again an operation that combines object-type and operation. I'd say
the object here is "row" and the operation is "insert", or, to be more general "new"
or "create".
We can have a "new" operation on a table and on a database, a trigger and an index.
Quite universal, no? We can have a "delete" operation on a table and a database and
a row and a column and all these types of objects we handle.

I mean if we had a good OOP structure, we could have a class called "Database", one
called "Table", one called "Row" and one called "Column". And we would call methods
on these like delete(), new() or rename(). Not all classes would have exactly the same
methods available, but many would be there for quite a few of them.
Then when we call these methods triggered by a GET parameter, it would be a helpful
to know what the type of the object is.

I mean it doesn't matter much for the current messy architecture. I agree that your
approach is better than the current one, but I'd say mine is more flexible and would
be helpful for a better future architecture.

> Two parameters only make for a more complicated set of ifs.
I wouldn't consider an if that has two simple isset()-conditions more complicated than
something like if(isset($_GET['page']) && strpos($_GET['page'],'table_')===0) which
your solution will easily end up with.

> Add this before the switch ;-)
> $page = $page ?: 'home';
Seriously? This fixes the problem that $page might be undefined? What happens with
this if $page is undefined? You'll get a PHP notice error. You'd need some isset()
either here or before, which would make this line of code completely useless because
you could initialize $page with "home" when you isset() on the parameter.

> [*] it would be nice if it were, but let's leave that for a separate issue
I agree with you on that one. If you want to have multiple tabs with different dbs,
having the Db stored in the session is not a good idea.

___

Okay. So these were my last arguments on this. As I neither have time nor interest
in implementing a change like this at the moment, feel free to implement your solution
if you like to. It is definitely better than the current one. I only fear we'll need
to change it again one day.

Reported by crazy4chrissi on 2012-11-23 20:43:44

[dreadnaut]

Same here, also because I think we are mainly not understanding each
other —your objections don't match with what I'm proposing (which means
my explanation was badly written) and my rebuttal falls flat.

I'll just try to clarify a couple of points, and then focus on something
else —one day I'll manage to explain everything properly.

This is what I find confusing in the current implementation: the 
operations have the same name, but then execute different actions 
because renaming a table is different than renaming a database. Thus we 
need to check two both the "object" (db/table) and the "method" (one of 
the actions above).

I think this might be the point: you already have an architecture in 
mind, based on a few classes, whose methods are called more or less 
directly using the page parameters.

E.g.  ?object=table&action=search&table=something&query=...

will roughly result in

$table = new Table('something');
$result = $table->search('...');

The thing is that what I am proposing is still compatible with such 
implementation, because it only involves the *output* routines, that is, 
what happens after you get $result. For example:

if ( Table::exists($table) )
{
   $table = new Table('something');
   $result = $table->search('...');
   $page = 'search_results';
} else {
   $message = "Table $table does not exist";
   $page = 'error';
}

switch($page)
{
   ...
}


The switch could also be replaced by a class, but I think that oop is a 
bit difficult at the moment, seeing the scope mess of global variables. 
If we split the code and sort that out, objects would become easier to 
write later.

Pseudo-code, come on, and there was a smiley! $page would always be 
undefined, because hopefully register_globals is off. I should have written

$page = isset($_GET['page']) ? $_GET['page'] : 'home';
if (!in_array($page, array('home', 'table_...', 'database_...')))
{
   header('Location: ' . PAGE);
   die();
}
switch ($page) {
  ...

or something similar, but I was lazy, sorry.


Hope I didn't waste too much of your time with this, and that I managed 
to clarify my point. I don't really need to implement my ideas, I'm 
opening these "issues" to discuss points I think are important, but it's 
quite likely that better solutions exist than the ones I suggest, and 
they can only come up discussing the issues :)

Reported by dreadnaut on 2012-11-23 21:45:01

[Christopher Kramer]

Okay.
> Pseudo-code, come on, and there was a smiley!
Sorry for that one...

I think it's good to discuss points like this, I mean we both can learn possible advantages
and disadvantages of approaches and express ourselves (which seems to need improvement
here and there...). I just thought we reached a point where we should stop the discussion
as it won't get us much further.

> I don't really need to implement my ideas
I really meant it seriously when I said feel free to implement it. It would be the
best option for you to prove me wrong if your approach will work great and clean even
with a future architecture ;-) And I am sure it is better than the current solution.

Reported by crazy4chrissi on 2012-11-23 21:54:11

[dreadnaut]

Reported by dreadnaut on 2012-11-27 20:12:56 - Labels added: Priority-Low, Maintainability - Labels removed: Priority-Medium

[dreadnaut]

I'm marking this as won't-fix, since I am not of the opinion that "things are not that
easy" and split-source will probably lead us in more interesting directions.

Reported by dreadnaut on 2013-03-13 20:33:07 - Status changed: WontFix