Proposal:GetSetVarRefactor
From Movable Type
Contents |
GetSetVarRefactor: A proposal to refactor template variable handlers
Abstract
Nearly all of the template variable manipulation tags are handled directly by one or both of two methods in MT::Template::ContextHandlers: _hdlr_set_var and _hdlr_get_var. These tags include:
- SetVarBlock
- SetVarTemplate
- Var
- GetVar
- SetVar
Because of this overloading, those two methods not only contain duplicate code but are incredibly complex, forwarding between each other based on the presence of certain tag arguments and containing many conditionals to provide differential results for each tag.
All of this complexity has produced "tunnel-vision code" which performs well but only for the tags it knows about and handles. The losers in the equation are any tags provided by plugins or even core tags which these methods don't handle — SetVars, SetHashVar and the setvar filter — which suffer from a lack of expected consistency in argument parsing and data manipulation features provided by the others.
These two methods and the tags that use them need to be refactored in order to provide a more consistent and powerful feature set to template designers and first-class treatment of plugins which are created to extend the core featureset of MT.
Problem Statements
The current code
- Contains duplicated code which increases the likelihood of bugs
- Tries to handle tags in a soup-to-nuts fashion leaving plugin developers with the choice of either wholesale copying of code or performing acrobatics of pre- and post-manipulation of data to mimic proper calls to the methods (e.g. manipulating the stash and tag arguments to make the core methods correctly handle its data)
- Hardcodes tag names in conditionals that provide differential processing and results for each without providing a path for arbitrary tags and other methods of manipulation/lookup
- Doesn't even consistently handle all of the template variable manipulation/lookup tags leading to inconsistencies like the lack of advanced parsing/interpolation of the setvar filter attribute value and the hash keys in SetHashVar.
Background
Easily one of the biggest enhancements in MT 4 is the handling of template variables. The tags listed above as well as mt:If, mt:Else(If), mt:Unless, mt:Loop and others have provided unparalelled control and power to template designers over their templates and data and have become *absolutely* critical to the proper functioning of not only the default templates but the applications admin interface itself.
It is perhaps because so much progress was made in such a short time that the system is unfortunately flawed. These flaws manifest themselves mainly in the form of bugs, infuriating feature inconsistencies (made worse by a lack of comprehensive documentation and examples) and lack of extensiblity.
Examples
As a single example of duplicate code (there are others), both methods contain the nearly the same attribute parsing code found below:
my $name = $args->{name} || $args->{var};
return $ctx->error(MT->translate(
"You used a [_1] tag without a valid name attribute.", "<MT" . $ctx->stash('tag') . ">" ))
unless defined $name;
my ($func, $key, $index, $value);
if ($name =~ m/^(\w+)\((.+)\)$/) {
$func = $1;
$name = $2;
} else {
$func = $args->{function} if exists $args->{function};
}
# pick off any {...} or [...] from the name.
if ($name =~ m/^(.+)([\[\{])(.+)[\]\}]$/) {
$name = $1;
my $br = $2;
my $ref = $3;
if ($ref =~ m/^\$(.+)/) {
$ref = $ctx->var($1);
$ref = chr(0) unless defined $ref;
}
$br eq '[' ? $index = $ref : $key = $ref;
} else {
$index = $args->{index} if exists $args->{index};
$key = $args->{key} if exists $args->{key};
}
if ($name =~ m/^\$/) {
$name = $ctx->var($name);
}
The following is a single example of code in which the methods assume to know what tags are being handled. If the "tag" variable in the stash isn't one of the following, the value of the template variable never gets set.
my $val = ;
my $data = $ctx->var($name);
if (($tag eq 'setvar') || ($tag eq 'var')) {
$val = defined $args->{value} ? $args->{value} : ;
} elsif ($tag eq 'setvarblock') {
$val = _hdlr_pass_tokens(@_);
return unless defined($val);
} elsif ($tag eq 'setvartemplate') {
$val = $ctx->stash('tokens');
return unless defined($val);
$val = bless $val, 'MT::Template::Tokens';
}
Discussion
At the most basic level, both methods perform the following main functions:
- Parse and appropriately dereference the arguments
- Get the value of the stored variable, if any
- Post-process the value, if requested/needed
- Store or output the value of the variable
Unfortunately, the overloading of these two methods by a number of different tags (Var, SetVar, GetVar, SetVarBlock, SetVarTemplate) has greatly increased their complexity and requires conditionalization to provide differential results for each tag, conditionalization that was not done with extensibility by non-core code in mind.
These method presume to know which tags use them and what each needs in terms of parsing arguments, retrieving/manipulating/storing/outputting the value of the variable. To provide maximum extensibility (which, I might add takes heat off of the core team for bug fixes and new features) the intelligence and business logic must be given back to the individual tag handlers.
There are two ways to solve this problem as I see it:
- Ditch the overloading and create individual handlers for each tag which call a set of modularized methods which perform necessary manipulation and lookups.
- Refactor the two methods (perhaps combining them), keeping the core tag handling and logic intact but better accommodating plugins by accepting code references or triggering callbacks for data lookup and manipulation tasks.
Frankly, it would seem to me that #1 would not only require a lower level of effort but also reduce the overall complexity of the system instead of increasing it. In effect, the individual core tag handlers would be given the responsibility of handling the business logic just like a plugin's tag handler must while the task-centric heavy lifting would be done by the modularized methods.
In either case, I beseech you to think like a plugin developer in doing this because these methods (like an unfortunately large percentage of the rest of the code base) are very core-centric making interaction with and extending of MT's core features much more difficult. Because of the importance of MT template variables and the tags with which they are tested and manipulated, it is essential that we set the ship upright as soon as possible. Doing so will produce happy and satisfied users and pave the way for a whole new set of abilities and feature provided by third-party plugins.
Requirements
- Consistency: Template variable lookup/testing/manipulation tags should act consistently and predictably in the eyes of the template designer
- Simplicity: Duplicate code should be abstracted out to reduce the possibilities of bugs and lower the maintenance cost of the code. The code should be modularized to reduce the size and performed tasks of each method. It should be simple to understand without having to juggle in your mind ten conditionals active over the course of four pages of code.
- Extensibility: It should be simple for a plugin developer to create new template variable-related template tags which act consistently with the core tags without resorting to duplication of dozens of lines of code or ridiculous argument/stash hackery. Preferably, this should be as easy as copying a similar-acting tag handler from the core code and modifying it to call the set of internal methods needed to process its args/data.
Effect on the platform/existing features/code
Making this change will do the following:
- Make Movable Type's templating language easier to understand and use
- Reduce the number of bugs in the related code and cost of maintenance to the core team
- Make developers lives easier and their plugins more powerful providing more features to the users without impacting the core development team
The only negative effect is the time it will take to refactor the code. However, it is my very firm belief that the time needed is far outstripped by the current and future load of related bugs on the core team. This, in fact, could be a time saver for the core team and provide immense benefits that build on themselves along the way.
[Author's note: Editing stopped here. All content below is stock template copy left for future use once the proposal is moving forward]
Project Plan
In this section, detail the approach that you and/or your project team will take to implementation. Unless your proposal covers a fairly short amount of time/effort, you may want to break it up into phases. This will allow you discreet, bite sized chunks of work to tackle in a series of sprints from one milestone to the next.
Implementation Notes
Talk about the details of implementation. For example, if you've planned for a phased approach, explain how you came to those milestones and what state the code is in between each.
If Your project requires additional or later versions of infrasturcture, mention those here as well.
Resources
This is a catchall section that serves as a "see also" list of external sites and resources as well as a holding place for assets internal to the project team. For example:
- Screenshots, comps, wireframes or other visual media to inform readers or project teammates
- Links to external resources (e.g. RFCs that document some important part of an external protocol)
- Links to similar products, plugin or systems for Movable Type or any other platform
Project Team
Below is a list of community members participating in the project. All users are linked to their wiki user page and project leaders are marked with an asterisk.
If you are interested in participating but still not sure, add yourself to the bottom of the list. You can italicize your name if you are interested but undecided.
Related Bugs
Please provide a link to any related bug in FogBugz.