Werdna has uploaded a new change for review.
https://gerrit.wikimedia.org/r/95583
Change subject: WIP: Flow output refactoring.
......................................................................
WIP: Flow output refactoring.
This is just a prototype to get feedback on the concept so I do not waste
three days writing something that nobody likes. Currently I've only ported
Timestamp and Post UIElements so people have an idea of how it looks.
The basic idea is that each element has a UIRenderer implementation which
generates HTML for it. An instance of the UIRenderer implementation is fully
instantiatedwith all parameters, and the HTML can be got by calling render().
In tandem with this, there is a UIRenderer implementation called UIElement,
which is a special type of UIRenderer that can be instantiated with a set
of known pre-defined parameters using the Templating object.
Finally, I added a postprocessing mechanism to Templating that allows you to
add a <flow-element> tag (I wanted to use namespaces but the DOM support in
PHP kept stripping them) and retrieve a UIElement subclass from the registry
in UI.php to render the element required. Unfortunately, and for obvious
reasons, you can only pass string parameters to these elements.
Together, I hope that these innovations can clean up our template files, and
improve the separation of concerns in our output code. However, I am sort of
aware that this is another case of NIH, so if anybody knows of a PHP
templating framework that would do a better job, then do let me know!
Change-Id: Id20a982a5e3efc69e92c125d108cd4c7f19b8069
---
M Flow.php
A UI.php
M container.php
R includes/Rendering/Elements/Post.php
A includes/Rendering/Elements/Timestamp.php
A includes/Rendering/TemplateRenderer.php
A includes/Rendering/UIRenderer.php
M includes/Templating.php
M templates/topic.html.php
9 files changed, 422 insertions(+), 69 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Flow
refs/changes/83/95583/1
diff --git a/Flow.php b/Flow.php
index a30eaf2..a9928ba 100755
--- a/Flow.php
+++ b/Flow.php
@@ -137,6 +137,12 @@
$wgAutoloadClasses['Flow\Block\TopicBlock'] = $dir .
'includes/Block/Topic.php';
$wgAutoloadClasses['Flow\Block\TopicView'] = $dir . 'includes/Block/Topic.php';
+// UI rendering stuff
+$wgAutoloadClasses['Flow\Rendering\UIRenderer'] = $dir .
'includes/Rendering/UIRenderer.php';
+$wgAutoloadClasses['Flow\Rendering\TemplateRenderer'] = $dir .
'includes/Rendering/TemplateRenderer.php';
+$wgAutoloadClasses['Flow\Rendering\UIElement'] = $dir .
'includes/Rendering/UIRenderer.php';
+$wgAutoloadClasses['Flow\Rendering\UIElementFactory'] = $dir .
'includes/Rendering/UIRenderer.php';
+
// API modules
$wgAutoloadClasses['ApiQueryFlow'] = "$dir/includes/api/ApiQueryFlow.php";
$wgAutoloadClasses['ApiParsoidUtilsFlow'] =
"$dir/includes/api/ApiParsoidUtilsFlow.php";
@@ -233,6 +239,9 @@
// Action details config file
require $dir . 'FlowActions.php';
+// UI element config file
+require $dir . 'UI.php';
+
// Register activity log formatter hooks
foreach( $wgFlowActions as $action => $options ) {
if ( isset( $options['log_type'] ) ) {
diff --git a/UI.php b/UI.php
new file mode 100644
index 0000000..19b7bf9
--- /dev/null
+++ b/UI.php
@@ -0,0 +1,15 @@
+<?php
+
+$wgAutoloadClasses += array(
+ 'Flow\Rendering\Timestamp' =>
__DIR__.'/includes/Rendering/Elements/Timestamp.php',
+ 'Flow\Rendering\Post' =>
__DIR__.'/includes/Rendering/Elements/Post.php',
+);
+
+$wgFlowUIElements = array(
+ 'timestamp' => array(
+ 'class' => 'Flow\Rendering\Timestamp',
+ ),
+ 'post' => array(
+ 'class' => 'Flow\Rendering\Post',
+ ),
+);
\ No newline at end of file
diff --git a/container.php b/container.php
index 528e0e2..6d637c1 100644
--- a/container.php
+++ b/container.php
@@ -393,4 +393,12 @@
);
} );
+$c['factory.uielement'] = $c->share( function( $c ) {
+ global $wgFlowUIElements;
+
+ return new Flow\Rendering\UIElementFactory(
+ $wgFlowUIElements
+ );
+} );
+
return $c;
diff --git a/includes/View/Post.php b/includes/Rendering/Elements/Post.php
similarity index 64%
rename from includes/View/Post.php
rename to includes/Rendering/Elements/Post.php
index 173a1df..e2bfe0d 100644
--- a/includes/View/Post.php
+++ b/includes/Rendering/Elements/Post.php
@@ -1,27 +1,82 @@
<?php
-namespace Flow\View;
+namespace Flow\Rendering;
+use Flow\Container;
use Flow\Model\PostRevision;
+use Flow\PostActionPermissions;
use Flow\Templating;
+use Flow\View\PostActionMenu;
use Linker;
use User;
-class Post {
+class Post extends TemplateRenderer {
protected $user;
protected $post;
protected $actions;
protected $creatorUserText;
- /**
- * @param User $user The User viewing posts
- */
- public function __construct( User $user, PostRevision $post,
PostActionMenu $actions ) {
- $this->user = $user;
- $this->post = $post;
- $this->actions = $actions;
+ // @todo This method has too many responsibilities
+ public function instantiate( array $parameters ) {
+ parent::instantiate( $parameters + array(
+ 'template' => 'flow:post.html.php',
+ ) );
+ global $wgFlowTokenSalt;
- $this->creatorUserText = $post->getCreatorName( $this->user );
+ $this->user = $parameters['user'];
+ $this->block = $parameters['block'];
+ $this->post = $parameters['post'];
+ $this->urlGenerator = $parameters['urlGenerator'];
+
+ if ( $this->post->isTopicTitle() ) {
+ throw new \MWException( 'Cannot render topic title with
' . __CLASS__ );
+ }
+
+ $this->creatorUserText = $this->post->getCreatorName(
$this->user );
+
+ // @todo pass this in as a parameter later
+ $actions = Container::get( 'flow_actions' );
+
+ $this->actions = new PostActionMenu(
+ $this->urlGenerator,
+ $actions,
+ new PostActionPermissions( $actions, $this->user ),
+ $this->block,
+ $this->post,
+ $this->user->getEditToken( $wgFlowTokenSalt )
+ );
+ }
+
+ public function getParameters() {
+ return parent::getParameters() + array(
+ 'postView' => $this,
+ );
+ }
+
+ public function getValidParameters() {
+ $params = parent::getValidParameters() + array(
+ 'urlGenerator' => array(
+ 'required' => true,
+ 'description' => 'A URLGenerator object',
+ ),
+ 'user' => array(
+ 'required' => true,
+ 'description' => 'The User who is viewing the
post',
+ ),
+ 'post' => array(
+ 'required' => true,
+ 'description' => 'The PostRevision object to
show',
+ ),
+ 'block' => array(
+ 'required' => true,
+ 'description' => 'The Block object that this
post is being shown in',
+ ),
+ );
+
+ // We handle this ourselves
+ unset( $params['template'] );
+
+ return $params;
}
public function replyPlaceholder() {
diff --git a/includes/Rendering/Elements/Timestamp.php
b/includes/Rendering/Elements/Timestamp.php
new file mode 100644
index 0000000..75b018b
--- /dev/null
+++ b/includes/Rendering/Elements/Timestamp.php
@@ -0,0 +1,78 @@
+<?php
+
+namespace Flow\Rendering;
+use Html;
+use MWTimestamp;
+
+class Timestamp extends UIElement {
+ public function instantiate( array $params ) {
+ extract( $params );
+
+ if ( $timestamp instanceof MWTimestamp ) {
+ $this->timestamp = $timestamp;
+ } else {
+ $this->timestamp = new MWTimestamp( $timestamp );
+ }
+
+ $this->historicalLink = $historicalLink;
+ $this->tag = $tag;
+ }
+
+ public function getValidParameters() {
+ return array(
+ 'timestamp' => array(
+ 'required' => true,
+ 'description' => 'The timestamp to render',
+ ),
+ 'historicalLink' => array(
+ 'description' => 'A URL to link the timestamp
to, if appropriate',
+ ),
+ 'tag' => array(
+ 'default' => 'p',
+ 'description' => 'The HTML tag to enclose the
timestamp in',
+ ),
+ );
+ }
+
+ public function render() {
+ $agoTime = Html::element(
+ 'span',
+ array(
+ 'style' => 'display: inline',
+ 'class' => 'flow-agotime',
+ ),
+ $this->timestamp->getHumanTimestamp()
+ );
+
+ $utcTime = Html::element(
+ 'span',
+ array(
+ 'style' => 'display: none',
+ 'class' => 'flow-utctime',
+ ),
+ $this->timestamp->getTimestamp( TS_RFC2822 )
+ );
+
+ $html = "$agoTime\n$utcTime";
+
+ if ( ! is_null( $this->historicalLink ) ) {
+ $html = Html::rawElement(
+ 'a',
+ array(
+ 'href' => $this->historicalLink
+ ),
+ $html
+ );
+ }
+
+ $html = Html::rawElement(
+ $this->tag,
+ array(
+ 'class' => 'flow-datestamp',
+ ),
+ $html
+ );
+
+ return $html;
+ }
+}
\ No newline at end of file
diff --git a/includes/Rendering/TemplateRenderer.php
b/includes/Rendering/TemplateRenderer.php
new file mode 100644
index 0000000..209c364
--- /dev/null
+++ b/includes/Rendering/TemplateRenderer.php
@@ -0,0 +1,37 @@
+<?php
+
+namespace Flow\Rendering;
+
+class TemplateRenderer extends UIElement {
+ protected $templating;
+ protected $template = null;
+ protected $params = array();
+
+ public function instantiate( array $parameters ) {
+ $this->templating = $parameters['templating'];
+ $this->template = $parameters['template'];
+ }
+
+ public function getValidParameters() {
+ return array(
+ 'templating' => array(
+ 'required' => true,
+ ),
+ 'template' => array(
+ 'required' => true,
+ ),
+ );
+ }
+
+ public function render( ) {
+ return $this->templating->render( $this->getTemplate(),
$this->getParameters() );
+ }
+
+ public function getTemplate() {
+ return $this->template;
+ }
+
+ public function getParams() {
+ return $this->params;
+ }
+}
\ No newline at end of file
diff --git a/includes/Rendering/UIRenderer.php
b/includes/Rendering/UIRenderer.php
new file mode 100644
index 0000000..d21bdbf
--- /dev/null
+++ b/includes/Rendering/UIRenderer.php
@@ -0,0 +1,122 @@
+<?php
+
+namespace Flow\Rendering;
+
+/**
+ * Interface for an object that generates HTML for a particular UI element.
+ *
+ * The basic semantics are like so: a fully instantiated UIRenderer represents
+ * a single instance of some renderable UI element, including containing all
+ * appropriate parameters.
+ *
+ * That is: any parameters are passed in instantiation, rather than in
+ * invocation.
+ */
+interface UIRenderer {
+ /**
+ * Returns the HTML output for this UI element.
+ * @return String|Null The HTML output.
+ */
+ function render();
+}
+
+/**
+ * Implementation of UIRenderer that is used in Templating for an element that
+ * can be instantiated and rendered in a standardised way.
+ *
+ * Therefore it has a stricter contract: it has to declare and accept
+ * parameters.
+ */
+abstract class UIElement implements UIRenderer {
+
+ /**
+ * Public-facing constructor.
+ * @param array $params Raw parameters
+ */
+ public function __construct( array $params ) {
+ $realParams = $this->processParameters( $params );
+
+ $this->parameters = $realParams;
+ $this->instantiate( $realParams );
+ }
+
+ protected function getParameters() {
+ return $this->parameters;
+ }
+
+ /**
+ * Takes in raw parameters, validates them, and applies
+ * default values.
+ * @param array $params Parameters passed by the caller.
+ * @return array Validated and cleaned parameters.
+ */
+ protected function processParameters( array $paramInput ) {
+ $validParams = $this->getValidParameters();
+ $realParams = array();
+ $params = array();
+
+ // Lowercase all param names
+ foreach( $paramInput as $key => $value ) {
+ $params[strtolower( $key )] = $value;
+ }
+
+ foreach( $validParams as $name => $info ) {
+ $lName = strtolower( $name );
+ if ( isset( $params[$lName] ) ) {
+ $realParams[$name] = $params[$lName];
+ } elseif ( isset( $info['required'] ) &&
$info['required'] ) {
+ throw new \MWException( "Parameter $name is
required" );
+ } elseif ( isset( $info['default'] ) ) {
+ $realParams[$name] = $info['default'];
+ } else {
+ $realParams[$name] = null;
+ }
+ }
+
+ return $realParams;
+ }
+
+ /**
+ * Sets up this UIElement from the given params.
+ *
+ * Guaranteed to be called from the constructor.
+ * @param array $params Parameter values, after being processed for
+ * default values and so on.
+ */
+ abstract protected function instantiate( array $params );
+
+ /**
+ * Returns a list of valid parameters for this UIElement
+ * @return array An associative array of valid parameters.
+ *
+ * Each entry has the following possible keys:
+ * default: The default value for this parameter.
+ * description: Optional description for this parameter.
+ * required: If set to true, this parameter is required.
+ */
+ abstract function getValidParameters();
+}
+
+class UIElementFactory {
+ function __construct( $elements ) {
+ $this->elements = $elements;
+ }
+
+ /**
+ * Instantiates a named UIElement
+ * @param string $elementName Key to $wgFlowUIElements
+ * @param array $params List of parameters
+ * @return UIElement A UIElement object ready to render
+ */
+ public function getElement( $elementName, array $params ) {
+ if ( ! isset( $this->elements[$elementName] ) ) {
+ throw new \MWException( "Invalid element key" );
+ }
+
+ $descriptor = $this->elements[$elementName];
+ $class = $descriptor['class'];
+ $element = new $class( $params + $descriptor );
+
+ return $element;
+ }
+}
\ No newline at end of file
diff --git a/includes/Templating.php b/includes/Templating.php
index 7217345..82b5a1d 100644
--- a/includes/Templating.php
+++ b/includes/Templating.php
@@ -8,8 +8,10 @@
use Flow\Model\PostRevision;
use Flow\Model\UUID;
use Flow\Model\Workflow;
+use Flow\Rendering\UIElement;
use OutputPage;
// These dont really belong here
+use DOMDocument;
use Html;
use Linker;
use MWTimestamp;
@@ -52,11 +54,54 @@
$content = ob_get_contents();
ob_end_clean();
+ $content = $this->postprocess( $content );
+
if ( $return ) {
return $content;
} else {
$this->output->addHTML( $content );
}
+ }
+
+ /**
+ * Postprocessing for a template call.
+ *
+ * Currently, just allows HTML to call a Flow UI Element.
+ * @param string $content HTML content
+ * @return string Postprocessed HTML
+ */
+ protected function postprocess( $content ) {
+ $content = mb_convert_encoding( $content, 'HTML-ENTITIES',
"UTF-8" );
+
+ // Sort of dirty hack to improve performance
+ if ( strpos( $content, '<flow-element' ) !== false ) {
+ $originalUseInternalErrors =
libxml_use_internal_errors( true );
+
+ $dom = new DOMDocument();
+ $dom->loadHTML( $content );
+
+ $embeddedElements = $dom->getElementsByTagName(
'flow-element' );
+
+ foreach( $embeddedElements as $element ) {
+ $params = array();
+
+ foreach( $element->attributes as $attr ) {
+ $params[$attr->nodeName] =
$attr->nodeValue;
+ }
+
+ $replacementHTML = $this->renderElement(
$params['elementname'], $params, true );
+ $replacementFragment =
$dom->createDocumentFragment();
+ $replacementFragment->appendXML(
$replacementHTML );
+
+ $element->parentNode->replaceChild(
$replacementFragment, $element );
+ }
+
+ $content = $dom->saveHTML();
+
+ libxml_use_internal_errors( $originalUseInternalErrors
);
+ }
+
+ return $content;
}
protected function applyNamespacing( $file ) {
@@ -82,6 +127,23 @@
// Everything below here *DOES* *NOT* belong in this class. Its also
pointless for us to invent a properly
// abstracted templating implementation so these can be elsewhere.
Figure out if we can transition to an
// industry standard templating solution and stop the NIH.
+
+ public function renderElement( $element, $parameters, $return = false )
{
+ $parameters += array(
+ 'templating' => $this,
+ 'urlGenerator' => $this->urlGenerator,
+ );
+
+ // @todo Pass to constructor instead once initial approach is
validated
+ $elementFactory = Container::get( 'factory.uielement' );
+ $html = $elementFactory->getElement( $element, $parameters
)->render();
+
+ if ( $return ) {
+ return $html;
+ } else {
+ $this->output->addHTML( $html );
+ }
+ }
public function getUrlGenerator() {
return $this->urlGenerator;
@@ -92,36 +154,12 @@
}
public function renderPost( PostRevision $post, Block $block, $return =
true ) {
- global $wgFlowTokenSalt;
-
- if ( $post->isTopicTitle() ) {
- throw new \MWException( 'Cannot render topic with ' .
__METHOD__ );
- }
-
- // @todo: I don't like container being pulled in here, improve
this some day
- $container = Container::getContainer();
-
- // An ideal world may pull this from the container, but for now
this is fine. This templating
- // class has too many responsibilities to keep receiving all
required objects in the constructor.
- $view = new View\Post(
- $container['user'],
- $post,
- new View\PostActionMenu(
- $this->urlGenerator,
- $container['flow_actions'],
- new PostActionPermissions(
$container['flow_actions'], $container['user'] ),
- $block,
- $post,
- $container['user']->getEditToken(
$wgFlowTokenSalt )
- )
- );
-
- return $this->render(
- 'flow:post.html.php',
+ return $this->renderElement( 'post',
array(
- 'block' => $block,
'post' => $post,
- 'postView' => $view,
+ 'block' => $block,
+ 'urlGenerator' => $this->urlGenerator,
+ 'user' => Container::get( 'user' ),
),
$return
);
diff --git a/templates/topic.html.php b/templates/topic.html.php
index 742533a..7b017bd 100644
--- a/templates/topic.html.php
+++ b/templates/topic.html.php
@@ -50,40 +50,31 @@
</div>
</div>
- <p class="flow-datestamp">
- <?php
- // timestamp html
- $content = '
- <span class="flow-agotime"
style="display: inline">' . htmlspecialchars(
$topic->getLastModifiedObj()->getHumanTimestamp() ) . '</span>
- <span class="flow-utctime"
style="display: none">' . htmlspecialchars(
$topic->getLastModifiedObj()->getTimestamp( TS_RFC2822 ) ) . '</span>';
+ <?php
+ $children = $root->getChildren();
- $children = $root->getChildren();
+ // Timestamp should be turned into a link to history if:
+ $history = false;
+ // topic title has changed
+ $history |= !$root->isFirstRevision();
+ // topic has more than 1 comment
+ $history |= count( $children ) > 1;
+ // first comment was submitted separately from topic
title
+ $history |= isset( $children[0] ) &&
$children[0]->getRevisionId()->getTimestamp() !==
$root->getRevisionId()->getTimestamp();
+ // original topic comment has replies
+ $history |= isset( $children[0] ) && count(
$children[0]->getChildren() ) > 0;
- // Timestamp should be turned into a link to
history if:
- $history = false;
- // topic title has changed
- $history |= !$root->isFirstRevision();
- // topic has more than 1 comment
- $history |= count( $children ) > 1;
- // first comment was submitted separately from
topic title
- $history |= isset( $children[0] ) &&
$children[0]->getRevisionId()->getTimestamp() !==
$root->getRevisionId()->getTimestamp();
- // original topic comment has replies
- $history |= isset( $children[0] ) && count(
$children[0]->getChildren() ) > 0;
+ if ( $history ) {
+ $historyUrl = $this->generateUrl(
$root->getPostId(), 'topic-history' );
+ } else {
+ $historyUrl = null;
+ }
+ ?>
- if ( $history ) {
- // build history button with timestamp
html as content
- echo Html::rawElement( 'a',
- array(
- 'class' =>
'flow-action-history-link',
- 'href' =>
$this->generateUrl( $root->getPostId(), 'topic-history' ),
- ),
- $content
- );
- } else {
- echo $content;
- }
- ?>
- </p>
+ <flow-element
+ elementName="timestamp"
+ timestamp="<?php echo htmlspecialchars(
$topic->getLastModifiedObj()->getTimestamp() ); ?>"
+ historicalLink="<?php echo $historyUrl; ?>" />
<?php
/*
--
To view, visit https://gerrit.wikimedia.org/r/95583
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id20a982a5e3efc69e92c125d108cd4c7f19b8069
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: Werdna <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
