Skip to content
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

Merged
merged 34 commits into from Jun 26, 2013
Merged

Controllers (part ...) #576

merged 34 commits into from Jun 26, 2013

Conversation

norv
Copy link
Contributor

@norv norv commented Jun 22, 2013

Another controllers PR for your attention. Its main purpose is addition of a base class for controllers.

  • introduction of an abstract base class for all controlers, Action_Controller. It simply declares action_index() and pre_dispatch(), methods that many may have already. (Almost) all controllers now extend Action_Controller.
  • made classes those who were not yet. Such as: Profile controllers (several of them), Xml, moderation iirc, etc.
  • reworked some ModerationCenter and around it; also methods/functions in Profile classes.
  • Xml controller had two 'types' of functions, those for previews, and random other xmlhttp calls. I split Xml controller into two, Xmlpreview and Xml. (with the obvious roles)
  • some bug fixes; small tweaks like method visibility.
  • moved to subs the database-working functions which serve lists, but created list_() callbacks for lists in 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).

* Defines an empty implementation for pre_dispatch()
* method.
*/
abstract class Action_Controller
Copy link
Member

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...

Copy link
Contributor Author

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?

Copy link
Member

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 ..

norv added 29 commits June 25, 2013 23:12
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>
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>
norv added 5 commits June 25, 2013 23:15
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>
@norv
Copy link
Contributor Author

norv commented Jun 25, 2013

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.

norv added a commit that referenced this pull request Jun 26, 2013
@norv norv merged commit 722cbc8 into elkarte:master Jun 26, 2013
norv referenced this pull request in Spuds/Elkarte Aug 26, 2013
! Some spelling fixed

Signed-off-by: Spuds <spuds@spudsdesign.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants