New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Controllers (part ...) #576
Conversation
* Defines an empty implementation for pre_dispatch() | ||
* method. | ||
*/ | ||
abstract class Action_Controller |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question of the day: Is this class designed to do something in the future? The current implemantation would be more of an interface instead of an abstract class...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few thoughts:
- if it were an interface, it'd force implementation of all methods - pre_dispatch() - for all controllers. Otherwise they remain abstract and PHP will bail. Some of them have pre_dispatch(), but it doesn't seem a must for all, atm?
- same goes for eventually settings() in admin controllers. Many have it, but not all.
With an interface all are forced to write something (be it something empty), with a base abstract class, we can add an empty implementation in the base class, and who cares and needs to override it, will; the rest will just inherit it with no change needed. - sometimes, during Ema's template_layers experiments, I've seen the possibility that an object of the template layers class could be an instance var in any controller, initialized when execution of action_something() starts, and used to add layers to. It'd have a place as protected in the base class. To note, this isn't an intention, it's a just a thought, but having a base class already will save us from making it then.
- sometimes, during earlier (well, middle-late, lol) work on Action class, I've been playing with an isAllowed in the base controller class, or a dispatch(). I'm not happy with the results, and I'd rather keep things plain and explicit, i.e. make in action_index manually what we need to make on subActions, using Action class. But in case a need appears more clearly than before, perhaps we can reconsider using the base class for some default implementation of controller dispatch.
- for Elk, templates and languages are simply procedural files. Even the 'engine' loading them, is our core functions in Load.php. With Ema's template layers, and perhaps with language files loading (Arantor had interesting comments on potential improvements), we might want a template engine object, and a language engine object, which - as above - a base controller could always initialize and be done with; all will just add/call stuff on them.
That said, I don't feel there is any issue at all to add an interface. Or, an extra-interface 😸 I'm just tempted to keep the object oriented model to its simplest; simplest which does the job with no burden.
An abstract base class is an interface conceptually, after all; but, it also allows defaults.
We could give up pre_dispatch(), or implement it in all controllers; or we can add an extra-interface (Action_Controller implements Controller). I'm just curious the other way around, why it'd be a "actual" interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late response...
I can see the benefits of an abstract parent class now .. the current scaffolding design reminds me of an interface (primarily because the functions are emtpy ) but a parent class (abstract or not) has definitely some benefits, at least it allows to easily extend all childs functionality at once ..
Signed-off-by: Norv <a.w.norv@gmail.com>
Document action_index() for each. Rename redundant 'extra'-word logs methods. (i.e. modlog->log) Signed-off-by: Norv <a.w.norv@gmail.com>
Add index where missing. Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
instance methods for the list, and database-working functions for the db work involved. Wip cleaning globals from subs (tweaks should be in list_() methods.) Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
(tests grumble :P) Signed-off-by: Norv <a.w.norv@gmail.com>
Tweak index and method visibilities. Signed-off-by: Norv <a.w.norv@gmail.com>
the entry method is called action_index(). Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
method. This will require quite a revamp. Signed-off-by: Norv <a.w.norv@gmail.com>
(it was even called from there, must've been buggy.) Signed-off-by: Norv <a.w.norv@gmail.com>
…troller. Signed-off-by: Norv <a.w.norv@gmail.com>
Move to boards subs getBoardNotificationsCount(). (uses db; other boards notifications functions are currently in boards subs). Few other cleaning up bits, methods visibilities. Signed-off-by: Norv <a.w.norv@gmail.com>
utility functions in subs (boards and topic... we don't have notifications subs?), and list_ methods in controller. Signed-off-by: Norv <a.w.norv@gmail.com>
Xml controller, the rest of xmlhttp. Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
…lers.) Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Move a query to subs. Clean up topic attributes functions. Signed-off-by: Norv <a.w.norv@gmail.com>
Move two queries to subs. Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
and clean some more the mod center controller. Signed-off-by: Norv <a.w.norv@gmail.com>
…tions. Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
Signed-off-by: Norv <a.w.norv@gmail.com>
I added several list_ callback methods updates, a couple of refactorings related to moving queries out of controllers (Topic, MoveTopic), moderation blocks reworked as controller methods using subs. |
! Some spelling fixed Signed-off-by: Spuds <spuds@spudsdesign.com>
Another controllers PR for your attention. Its main purpose is addition of a base class for controllers.
Please note on this detail on createList() callbacks: list_ callbacks (i.e. list_getWarnedUsersCount or so) serve the controller which creates the list. But, they work with the database to get their data. I made list_ callback methods, in controllers, to be used by their list creation code; and, moved the actual database work in functions in subs. Please see: https://github.com/norv/elkarte/blob/6a90c1eb17878edfcbbb109f8692d9ae473cbb01/sources/controllers/ModerationCenter.controller.php#L1457 or norv@6a90c1e#L0R1560.
The list_ methods will call the subs function to get the data, eventually passing them params instead of globals. IMO, the least advantage is that watchUsersCount() etc, can be cached, tweaked, and reused, from subs, as expected (it's watched users count, a plain information), without being at all tied to some particular lists (those just use them too).
Please note on Action_Controller: action_index() is an abstract method. So, it makes official (lol) that every controller has an action_index() to begin with. Most already did, anyway (or had something as default entry), we've had an "unspoken" design choice, now they all do.
action_index() is called when there isn't a ?sa= specified, or even if there is: a default point of entry, from where the controller will forward to the right method. Just like our olde index.php is the point of entry in the whole app, action_index is (supposed to be) the point of entry in the controller class for the requests it handles. It allows to centralize things like permissions checks (common, usually), for admin controllers for example it was already done this way.
Additional note: most of changes in this PR are with Profile, some moderation I think, and the split of Xml controller. The rest of files have received a base class and at most a first method. I can't be sure, but I don't expect significant issues with conflicts. Let me know if you need time or tweaks or else, though. There are a couple missing (I'll add them later, if you don't first).