Skip to main content
Topic: Themes.php question (Read 5181 times) previous topic - next topic
0 Members and 1 Guest are viewing this topic.

Themes.php question

I'm refactoring Themes.php right now and need some input:

1) There are methods named action_edit_style, action_edit_, action_edit_template... These are mainly forwarders for action_edit();
I'd make them private, but would need some feedback regarding method names..
simply _action_edit_style, or _edit_style?

2) prepareThemeEditContext() is currently a function and is doing prechecks for action_edit()..
Question: move it to subs or make it a private method of the Themes_Controller?


Thorsten "TE" Eurich
------------------------

Re: Themes.php question

Reply #1

Ah well. I keep changing my mind about certain admin actions separation/naming/design, because there are several levels through which the user request is routed. 4 levels: action > area > sa > activity. (named activity in maintenance pages, not named any way elsewhere, but it happens here too). 4 levels is a lot (I'd gladly cut it down), but it is the case of admin actions.

To reconsider (my own first answer):
The browse and list actions are actions, and IMO very independent. It makes sense to directly (now) go as far as setting 'sa' for them (without passthrough edit) because they don't "edit" anything. So action_browse and action_list, or (ETA) since it's a conflict, well, action_themelist. This is a bit inconsistent with the menu/UI, because that 'forgets' we're in the 'edit' sub-section of themes. But keep-it-stupid-simple is also important. :)
If sa=browse seems a little too simplified, then sa=edit;activity=browse is IMO better anyway, than hardcoded conditions = guesswork of what is the action the user is doing. (i.e. haz filename? doesn't it? These should be checks of validity/security, not criteria to find out what is the action handler to execute.).

The other three (without submit). My question atm is if they are independent actions. (in the sense handled by controllers). I'd ask the question as: are those user actions on one page or on three pages? There are clear differences between editing css with previews, and validating php, etc, and three sub-templates for those pages already, for that matter (template_pagename()). So, I'd say they are three "action
" methods; action_ is what we have for handling user requests from/for different pages. They don't even depend on one another, the request is either for one or another.
(they shouldn't have that parameter though. No action_ should.)

Making them all private (in case we keep the forwarding), makes sense IMO, to avoid mistaken calls - they're not supposed to be called from anywhere else, so needlessly allowing it isn't very good.

Quote2) prepareThemeEditContext() is currently a function and is doing prechecks for action_edit()..
Question: move it to subs or make it a private method of the Themes_Controller?
A controller internal helper method: it's job is to make some security checks and cleanups, and it only wants to do what it says, to prepare $context. In a sense, that's what a controller is supposed to do: prepare $context with data for the template. I see it's used nowhere else, but by a single action of this.
The best moment for testing your PR is right after you merge it. Can't miss with that one.

Re: Themes.php question

Reply #2

yay, thanks for the details..
sa=browse (former action_edit_browse) and sa=themelist (formerly known as action_edit_list ) almost done :) Just need to find  a solution for the activate button in GenericMenu.template.php  (Should IMO be "Modify Themes" for all of them: sa=edit, sa=themelist and sa=browse)
Thorsten "TE" Eurich
------------------------

Re: Themes.php question

Reply #3

activate button? Sorry, I need to see the code, I'm afraid I'm not following well enough otherwise: I think it makes sense to be the same, but why is it a problem?
The best moment for testing your PR is right after you merge it. Can't miss with that one.

Re: Themes.php question

Reply #4

Quote from: TestMonkey – activate button? Sorry, I need to see the code, I'm afraid I'm not following well enough otherwise: I think it makes sense to be the same, but why is it a problem?
Don't worry, I was just talking to myself .. ;)
Thorsten "TE" Eurich
------------------------