Unify view/action handling for database and table pages
Issue #155
wontfix
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
Comments (12)
-
-
reporter 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 -
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 -
reporter 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 -
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 -
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 -
reporter 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 -
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 -
reporter 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 -
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 -
reporter Reported by
dreadnaut
on 2012-11-27 20:12:56 - Labels added: Priority-Low, Maintainability - Labels removed: Priority-Medium -
reporter 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
- Log in to comment
Reported by
crazy4chrissi
on 2012-11-22 23:29:06