Ticket #1934 (assigned enhancement)

Opened 14 months ago

Last modified 6 months ago

Improve SSViewer

Reported by: wakeless Owned by: mrickerby
Priority: medium Milestone:
Component: Sapphire Framework Version:
Severity: medium effort / impact Keywords: ssviewer templates syntax notformerge
Cc: michael@… Hours:

Description

Currently in SSViewer you can only access the variables on the current object.

SSViewer has a hard limit regarding how many functions you can string together.

SSViewer is not very hackable due to it's large use of complex regexes.

Attachments

SSContext.php (1.5 kB) - added by wakeless 14 months ago.
Context stack for viewer patch
ssviewer.diff (31.1 kB) - added by wakeless 14 months ago.
SSViewer patch, uses SSContext and moves away from the regexes

Change History

Changed 14 months ago by wakeless

Context stack for viewer patch

Changed 14 months ago by wakeless

SSViewer patch, uses SSContext and moves away from the regexes

Changed 13 months ago by sminnee

  • milestone set to 2.2.2 feature-lock

The main issue with this implementation is that some of the parsing is now done on every template execution, rather than at compile time. I think that we would want to still make a parser that spat out similar PHP code to the current parser.

This would probably be done with 2 phases of parsing: 1 to break the template down into the main control tokens, and another to parse each token. The 2nd stage would be a modified version of SSContext::evaluate(), outputting PHP code to be executed rather than processing it on the fly.

We want to navigate through the contexts using $Up and $Top. It would be very beneficial if Up() and Top() could be defined on the ViewableData? class, as that way you could refer to them from your code as well as from the templates. This could probably be done by calling an $item->setContext() at the top of the template and inside each <% control %> loop.

In ViewableData?, we could define some fairly simple functions like so:

function Up() {
  return $this->context->getParent($this);
}
function Top() {
  return $this->context->getTop($this);
}

In order for $Up.Up.Up.Title to work, we would need to ensure that different context objects were applied to each level, or that the context had a way of knowing where the request was coming from - hence passing $this to getParent() and getTop().

All of this leads to a pretty radical redevelopment of the code provided in the patch. Perhaps it would be better for us to start from scratch and use the patch as "inspiration"?

Changed 13 months ago by sminnee

Other notes on the development of the parser:

Alternatively, we'd need to code up a system to check for matched pairs of <% control %> and <% end_control %> blocks, and avoid things like <% control XX %> <% if YY %> <% end_control %> <% end_if %> One potential approach with this would be to break the code into an sequential array, each item of which would contain one of:

  • control or end_control code
  • if or end_if code
  • $ variable
  • HTML literal

It would then be relatively straightforward to look ahead in the array and verify that there is a matching end_ code down the line.

Changed 13 months ago by wakeless

I seemed to have stopped getting emails from Trac... a tad annoying.

Anyway, I think the Up and Down functions should not be defined in ViewableData?, but as part of the templating language, realistically, ViewableData? should have no idea of what context it is being displayed in (it shouldn't even know that it is currently in a template). Now I'm not sure how to do this at the moment (maybe a <% Up %> ) or something like it.

Changed 13 months ago by wakeless

Also, Why are we worried about the templates being executed every time the page is loaded? I suppose it's because of speed, but I think the simplicity on the SSexecuter and the fact that we will be able to provide a heap better details on errors would outweigh the (IMHO) minimal performance gains.

There's places we can provide a heap better performance gains throughout Sapphire.

Those regexes are possibly the scariest thing in the whole codebase.

Changed 12 months ago by wakeless

  • cc michael@… added

Changed 12 months ago by wakeless

  • summary changed from Improve SSViewewr to Improve SSViewer

I think we should also look at providing a better interface for the $Layout and $Content constructs, at the moment it's not very flexible (in that it just looks for a template of the same name in the Layout/Content folders). If we could specify possibly some sort of array of values that these commands move up through.

That's not making much sense.

Currently a lot of our site templates have a Page.ss or XEPage.ss file that is the core of the layout, if we have another controller (not derived form Page or XEPage) that needs to use it, we need to overide the Layout or Content function and renderWith('Page'). Which doesn't really feel like the "right" way of going about it.

Changed 11 months ago by sminnee

Wakeless - it sounds like the issue you're having is that youre "template cascade" is different from your inheritance hierarchy. Perhaps we should have a method templateCascade(), that returns the set of templates to try. You could then overload templateCascade()?

In other news, Mark Rickerby is currently looking at using a context-free grammar for parsing .SS files, which should work very well.

Changed 11 months ago by wakeless

Care to get Mark (or you) to elaborate on how it is going to work?

Changed 11 months ago by sminnee

  • milestone changed from 2.2.2 feature-lock to 2.2.3 feature-lock

Changed 11 months ago by mrickerby

  • keywords syntax added

What specifically would you like to see elaborated? I'd be interested to know what you mean by SSViewer not being 'hackable' enough... Is there something important missing, or have you got ideas for future improvements?

Currently, I'm working on replacing the existing regex stripping functionality with a generated scanner that does the substitutions on a token by token basis. Once that's done, then we can revisit the concept of integrating a context stack, and jumping Up and Top which I think is fairly important functionality to have.

Changed 11 months ago by wakeless

When I said SSViewer wasn't 'hackable' enough, I mean that diving into it, say to find a bug or to add a feature there is an extremely harsh learning curve at the moment, due to the huge use of regexes, and quite a procedural style of code.

It looks like though you are taking pretty much the same approach I was (I've since not gotten time to finish the work I started but it's in the patches attached to this bug).

Changed 11 months ago by sminnee

  • keywords notformerge added

This will provide a useful guide to the CFG parser work but I don't think it's appropriate for directly merging in.

Changed 6 months ago by mrickerby

  • owner changed from sminnee to mrickerby
  • milestone 2.2.3 deleted

Changed 6 months ago by mrickerby

  • status changed from new to assigned
Note: See TracTickets for help on using tickets.