EBernhardson (WMF) has uploaded a new change for review. https://gerrit.wikimedia.org/r/78186
Change subject: various updates ...................................................................... various updates Rename AbstractIndex -> FeatureIndex Rename UniqueIndex -> UniqueFeatureIndex Rename SecondaryIndex -> TopKIndex Initial work pulling in visualeditor JS Add render method to Block interface Remove some now-irrelivant commenting ObjectStorage interface now extends from IteratorAggregate, various arn't fully implementing it yet and throw exceptions when called. More consistent error results, use null instead of false for consistency Rename lifecycle events, onPostLoad -> onAfterLoad and all similar, to prevent confusion between Post's to a topic and events that happen after something Change-Id: If85dbc1a9c8558e4ec0f5b8c2030f3e08b5dc399 --- M Flow.php M container.php M includes/Block/Block.php M includes/Block/Topic.php M includes/Block/TopicList.php M includes/Data/MultiDimArray.php M includes/Data/ObjectManager.php M includes/Data/RevisionStorage.php M includes/Model/AbstractRevision.php M includes/Model/PostRevision.php M includes/Model/Summary.php M includes/Model/Workflow.php M includes/Repository/SelectQueryBuilder.php M includes/Templating.php M includes/UrlGenerator.php A modules/base/ext.flow.base.js M special/SpecialFlow.php M vendor/Pimple.php 18 files changed, 246 insertions(+), 204 deletions(-) git pull ssh://gerrit.wikimedia.org:29418/mediawiki/extensions/Flow refs/changes/86/78186/1 diff --git a/Flow.php b/Flow.php index e660276..182b860 100755 --- a/Flow.php +++ b/Flow.php @@ -89,13 +89,6 @@ $wgAutoloadClasses['Flow\Data\PostRevisionStorage'] = $dir . 'includes/Data/RevisionStorage.php'; $wgAutoloadClasses['Flow\Data\SummaryRevisionStorage'] = $dir . 'includes/Data/RevisionStorage.php'; -// Classes that control user workflows -$wgAutoloadClasses['Flow\Controller\Controller'] = $dir . 'includes/Controller/Controller.php'; -$wgAutoloadClasses['Flow\Controller\DefaultController'] = $dir . 'includes/Controller/DefaultController.php'; -$wgAutoloadClasses['Flow\Controller\SubscriptionController'] = $dir . 'includes/Controller/SubscriptionController.php'; -$wgAutoloadClasses['Flow\Controller\DiscussionController'] = $dir . 'includes/Controller/DiscussionController.php'; -$wgAutoloadClasses['Flow\Controller\TopicController'] = $dir . 'includes/Controller/TopicController.php'; - // The individual workflow pieces $wgAutoloadClasses['Flow\Block\Block'] = $dir . 'includes/Block/Block.php'; $wgAutoloadClasses['Flow\Block\AbstractBlock'] = $dir . 'includes/Block/Block.php'; @@ -131,6 +124,7 @@ 'styles' => 'base/ext.flow.base.css', 'scripts' => 'base/ext.flow.base.js', 'dependencies' => array( + 'ext.visualEditor.standalone', ), 'messages' => array( ), diff --git a/container.php b/container.php index b0252ab..5f15341 100644 --- a/container.php +++ b/container.php @@ -49,8 +49,8 @@ use Flow\Data\BasicDbStorage; use Flow\Data\PostRevisionStorage; use Flow\Data\SummaryRevisionStorage; -use Flow\Data\UniqueIndex; -use Flow\Data\SecondaryIndex; +use Flow\Data\UniqueFeatureIndex; +use Flow\Data\TopKIndex; use Flow\Data\ObjectMapper; use Flow\Data\ObjectManager; @@ -71,8 +71,8 @@ array( 'definition_id' ) ); $indexes = array( - new UniqueIndex( $cache, $storage, 'flow_definition:pk', array( 'definition_id' ) ), - new UniqueIndex( $cache, $storage, 'flow_definition:name', array( 'definition_wiki', 'definition_name' ) ), + new UniqueFeatureIndex( $cache, $storage, 'flow_definition:pk', array( 'definition_id' ) ), + new UniqueFeatureIndex( $cache, $storage, 'flow_definition:name', array( 'definition_wiki', 'definition_name' ) ), ); return new ObjectManager( $mapper, $storage, $indexes ); @@ -87,56 +87,51 @@ // pk array( 'workflow_id' ) ); - $pk = new UniqueIndex( $cache, $storage, 'flow_workflow:pk', array( 'workflow_id' ) ); + $pk = new UniqueFeatureIndex( $cache, $storage, 'flow_workflow:pk', array( 'workflow_id' ) ); $indexes = array( $pk, // This is actually a unique index, but it wants the shallow functionality. - new SecondaryIndex( + new TopKIndex( $cache, $storage, 'flow_workflow:title', array( 'workflow_wiki', 'workflow_page_id', 'workflow_definition_id' ), array( 'shallow' => $pk, 'limit' => 1, 'sort' => 'workflow_id' ) ), ); - // not ready yet + //ch /valulds t not ready yet $lifecycle = array(); // $c['storage.user_subs.user_index'] ); return new ObjectManager( $mapper, $storage, $indexes, $lifecycle ); } ); // Arbitrary bit of revisioned wiki-text attached to a workflow $c['storage.summary'] = $c->share( function( $c ) { + global $wgFlowExternalStore; + $cache = $c['memcache.buffered']; $mapper = BasicObjectMapper::model( 'Flow\\Model\\Summary' ); - $storage = new SummaryRevisionStorage( $c['db.factory'] ); + $storage = new SummaryRevisionStorage( $c['db.factory'], $wgFlowExternalStore ); + + $workflowIndexOptions = array( + 'sort' => 'rev_id', + 'order' => 'DESC', + //'shallow' => array( 'rev_id' ), + 'create' => function( array $row ) { + return $row['rev_parent_id'] === null; + }, + ); $indexes = array( - new UniqueIndex( + new UniqueFeatureIndex( $cache, $storage, 'flow_summary:pk', array( 'rev_id' ) ), - new SecondaryIndex( + new TopKIndex( $cache, $storage, 'flow_summary:workflow', array( 'summary_workflow_id' ), - array( - 'limit' => 100, - 'sort' => 'rev_id', - 'order' => 'DESC', - //'shallow' => array( 'rev_id' ), - 'create' => function( array $row ) { - return $row['rev_parent_id'] === null; - }, - ) + array( 'limit' => 100 ) + $workflowIndexOptions ), - new SecondaryIndex( + new TopKIndex( $cache, $storage, 'flow_summary:latest', array( 'summary_workflow_id' ), - array( - 'limit' => 1, - 'sort' => 'rev_id', - 'order' => 'DESC', - //'shallow' => array( 'rev_id' ), - 'create' => function( array $row ) { - return $row['rev_parent_id'] === null; - }, - ) + array( 'limit' => 1 ) + $workflowIndexOptions ), ); @@ -154,12 +149,12 @@ array( 'topic_list_id', 'topic_id' ) ); $indexes = array( - new SecondaryIndex( + new TopKIndex( $cache, $storage, 'flow_topic_list:list', array( 'topic_list_id' ), array( 'sort' => 'topic_id' ) ), - new UniqueIndex( + new UniqueFeatureIndex( $cache, $storage, 'flow_topic_list:topic', array( 'topic_id' ) ), @@ -169,13 +164,14 @@ } ); // Individual post within a topic workflow $c['storage.post'] = $c->share( function( $c ) { + global $wgFlowExternalStore; $cache = $c['memcache.buffered']; $treeRepo = $c['repository.tree']; $mapper = BasicObjectMapper::model( 'Flow\\Model\\PostRevision' ); - $storage = new PostRevisionStorage( $c['db.factory'], $treeRepo ); + $storage = new PostRevisionStorage( $c['db.factory'], $wgFlowExternalStore, $treeRepo ); $indexes = array( - new UniqueIndex( $cache, $storage, 'flow_revision:pk', array( 'rev_id' ) ), - new SecondaryIndex( $cache, $storage, 'flow_revision:descendant', + new UniqueFeatureIndex( $cache, $storage, 'flow_revision:pk', array( 'rev_id' ) ), + new TopKIndex( $cache, $storage, 'flow_revision:descendant', array( 'tree_rev_descendant' ), array( 'limit' => 100, @@ -187,7 +183,7 @@ return $row['rev_parent_id'] === null; }, ) ), - new SecondaryIndex( $cache, $storage, 'flow_revision:latest_post', + new TopKIndex( $cache, $storage, 'flow_revision:latest_post', array( 'tree_rev_descendant' ), array( 'limit' => 1, @@ -205,7 +201,6 @@ // Storage implementation for user subscriptions, separate from storage.user_subs so it // can be used in storage.user_subs.user_index as well. $c['storage.user_subs.backing'] = $c->share( function( $c ) { - return new BasicDbStorage( // factory and table $c['db.factory'], 'flow_user_sub', @@ -215,12 +210,12 @@ } ); // Needs to be separate from storage.user_subs so it can be attached to Workflow updates // Limits users to 2000 subscriptions -// TODO: Can't use SecondaryIndex, needs to be custom. so it stores the right data +// TODO: Can't use TopKIndex, needs to be custom. so it stores the right data // TODO: Storage wont work either, it $c['storage.user_subs.user_index'] = $c->share( function( $c ) { $cache = $c['memcache.buffered']; $storage = $c['storage.user_subs.backing']; - return new SecondaryIndex( + return new TopKIndex( $cache, $storage, 'flow_user_sub:user', array( 'subscrip_user_id' ), array( 'limit' => 2000, 'sort' => 'subscrip_last_updated' ) ); @@ -243,12 +238,16 @@ array( 'Flow\\Model\\Definition' => 'storage.definition', 'Definition' => 'storage.definition', + 'Flow\\Model\\Workflow' => 'storage.workflow', 'Workflow' => 'storage.workflow', + 'Flow\\Model\\PostRevision' => 'storage.post', 'PostRevision' => 'storage.post', + 'Flow\\Model\\TopicListEntry' => 'storage.topic_list', 'TopicListEntry' => 'storage.topic_list', + 'Flow\\Model\\Summary' => 'storage.summary', 'Summary' => 'storage.summary', ) diff --git a/includes/Block/Block.php b/includes/Block/Block.php index bf8adc0..4605606 100644 --- a/includes/Block/Block.php +++ b/includes/Block/Block.php @@ -22,6 +22,12 @@ function commit(); /** + * Load whatever is necessary for rendering an use $templating to + * render it. + */ + function render( Templating $templating, array $options ); + + /** * @return string Unique name among all blocks on an object */ function getName(); @@ -52,7 +58,6 @@ } public function onSubmit( $action, User $user, array $data ) { - // Only one action currently if ( false === array_search( $action, $this->supportedActions ) ) { return null; } diff --git a/includes/Block/Topic.php b/includes/Block/Topic.php index f373aad..9c0f18a 100644 --- a/includes/Block/Topic.php +++ b/includes/Block/Topic.php @@ -196,6 +196,9 @@ 'history' => $history, ), $return ); } + + $templating->getOutput()->addModules( 'ext.flow.base' ); + return $templating->render( "flow:topic.html.php", array( 'block' => $this, 'topic' => $this->workflow, @@ -208,9 +211,6 @@ return $this->root; } return $this->root = $this->rootLoader->get( $this->workflow->getId() ); - } - - protected function loadPostHistory() { } // Somehow the template has to know which post the errors go with diff --git a/includes/Block/TopicList.php b/includes/Block/TopicList.php index e90a538..504523c 100644 --- a/includes/Block/TopicList.php +++ b/includes/Block/TopicList.php @@ -45,7 +45,8 @@ } $topicWorkflow = Workflow::create( $topicDef, $this->user, $this->workflow->getArticleTitle() ); - // Should we really have a top level post for the topic title? + // Should we really have a top level post for the topic title? Simplifies allowing + // a revisioned title. $topicPost = PostRevision::create( $topicWorkflow, $this->submitted['topic'] ); $firstPost = $topicPost->reply( $this->user, $this->submitted['content'] ); $topicListEntry = TopicListEntry::create( $this->workflow, $topicWorkflow ); @@ -57,23 +58,11 @@ } public function render( Templating $templating, array $options ) { - $topics = array(); - // New workflows cant have content yet - if ( !$this->workflow->isNew() ) { - $topicList = $this->storage->find( 'TopicListEntry', array( - 'topic_list_id' => $this->workflow->getId(), - ) ); - if ( $topicList ) { - foreach( $topicList as $entry ) { - $topicIds[] = $entry->getId(); - } - $roots = $this->rootLoader->getMulti( $topicIds ); - foreach ( $this->storage->getMulti( 'Workflow', $topicIds ) as $workflow ) { - $id = $workflow->getId(); - $topics[$id->getHex()] = new TopicBlock( $workflow, $this->storage, $roots[$id->getHex()] ); - } - } + if ( $this->workflow->isNew() ) { + $topics = array(); + } else { + $topics = $this->loadAllRelatedTopics(); } $templating->render( "flow:topiclist.html.php", array( @@ -85,5 +74,26 @@ public function getName() { return 'topic_list'; } + + protected function loadAllRelatedTopics() { + $found = $this->storage->find( 'TopicListEntry', array( + 'topic_list_id' => $this->workflow->getId(), + ) ); + if ( !$found ) { + return array(); + } + + $topics = array(); + foreach( $found as $entry ) { + $topicIds[] = $entry->getId(); + } + $roots = $this->rootLoader->getMulti( $topicIds ); + foreach ( $this->storage->getMulti( 'Workflow', $topicIds ) as $workflow ) { + $hexId = $workflow->getId()->getHex(); + $topics[$hexId] = new TopicBlock( $workflow, $this->storage, $roots[$hexId] ); + } + + return $topics; + } } diff --git a/includes/Data/MultiDimArray.php b/includes/Data/MultiDimArray.php index 936ebb3..bde44e7 100644 --- a/includes/Data/MultiDimArray.php +++ b/includes/Data/MultiDimArray.php @@ -81,7 +81,7 @@ // Maps from the query array to its position in the query array protected $queryKeys; protected $queryMap; - protected $queries; + protected $queries = array(); protected $result; public function __construct( array $queryKeys, $dimensions ) { @@ -94,7 +94,16 @@ // Add a query and its position. Positions must be unique. public function add( $query, $position ) { + $dim = count( (array) $position ); + if ( $dim !== $this->dimensions ) { + throw new \InvalidArgumentException( "Expection position with {$this->dimensions} dimensions, received $dim" ); + } $query = ObjectManager::splitFromRow( $query, $this->queryKeys ); + if ( $query === null ) { + // the queryKeys are either unset or null, and not indexable + // TODO: what should happen here? + return; + } $this->desiredOrder[$position] = $query; if ( !isset( $this->queryMap[$query] ) ) { $this->queries[] = $query; @@ -105,6 +114,11 @@ // merge a query into the result set public function merge( array $query, array $result ) { $query = ObjectManager::splitFromRow( $query, $this->queryKeys ); + if ( $query === null ) { + // the queryKeys are either unset or null, and not indexable + // TODO: what should happen here? + return; + } $this->result[$query] = $result; } @@ -124,8 +138,6 @@ if ( $dimensions > 1 ) { $final[$position] = self::sortResult( $query, $result, $dimensions - 1 ); } elseif ( !isset( $result[$query] ) ) { - throw new \Exception( 'Missing result' ); - } else { $final[$position] = $result[$query]; } } diff --git a/includes/Data/ObjectManager.php b/includes/Data/ObjectManager.php index c6f7242..76efa71 100644 --- a/includes/Data/ObjectManager.php +++ b/includes/Data/ObjectManager.php @@ -15,28 +15,25 @@ // - But we want to be able to replace that cache with a buffered cache that flushes // on db commit. // - Perhaps one buffered cache could wrap both redis and memcache? seems odd though -// Different indexes on the same ObjectManager will always use the same storage -// TODO: This interface is too tied to the ObjectManager, it should -// be possible to use things implementing LifecycleHandler without -// an ObjectManager instances(to keep things simpler). interface LifecycleHandler { - function onPostLoad( $object, array $old ); - function onPostInsert( $object, array $new ); - function onPostUpdate( $object, array $old, array $new ); - function onPostRemove( $object, array $old ); + function onAfterLoad( $object, array $old ); + function onAfterInsert( $object, array $new ); + function onAfterUpdate( $object, array $old, array $new ); + function onAfterRemove( $object, array $old ); } // Some denormalized data doesnt accept writes, it merely triggers cache updates // when something else does the write. Indexes are the primary use case. -interface ObjectStorage { +// IteratorAggregate rather than traversable to simplify nested implementations +interface ObjectStorage extends \IteratorAggregate { function find( array $attributes, array $options = array() ); /** * The BagOStuff interface returns with keys matching the key, unfortunatly - * we deal with composite keys which makes that awkward. Instead all findMulti - * implementations must return their result as if it was arrap_map( array( $obj, 'find' ), $queries ). - * This is necessary so result sets stay ordered - * + * we deal with composite keys which makes that awkward. Instead all + * findMulti implementations return their data with the same array indexes + * as the query in $queries. The order the requested queries are returned in + * is arbitrary. * * @param array $queries list of queries to perform * @param array $options Options to use for all queries @@ -44,6 +41,9 @@ */ function findMulti( array $queries, array $options = array() ); function getPrimaryKeyColumns(); + // Clear any information stored about loaded objects + // This interface is used by the frontend (ObjectLocator) and the backend (BasicDbStorage, etc) + //function clear(); } // Backing stores, typically in SQL @@ -53,7 +53,6 @@ function remove( array $row ); } -// Perhaps better names, because this isnt php serialization, its domain model <-> db row interface ObjectMapper { /** * Convert $object from the domain model to its db row @@ -132,7 +131,6 @@ * Denormalized indexes that are query-only. The indexes used here must * be provided to some ObjectManager as a lifecycleHandler to receive * update events. - * * Error handling is all wrong, but simplifies prototyping. */ class ObjectLocator implements ObjectStorage { @@ -153,6 +151,9 @@ return $result ? reset( $result ) : null; } + public function getIterator() { + throw new \Exception( 'Not Implemented' ); + } /** * All queries must be against the same index. Results are equivilent to * array_map, maintaining order and key relationship between input $queries @@ -175,8 +176,8 @@ $limit = false; } - $res = $this->getIndexFor( $keys, $options )->findMulti( $queries, $options ); - if ( !$res ) { + $res = $this->getIndexFor( $keys, $options )->findMulti( $queries ); + if ( $res === null ) { return null; } $retval = array(); @@ -217,7 +218,7 @@ // to be consistent. undo that for a flat result array $res = $this->findMulti( $queries ); if ( !$res ) { - return false; + return null; } $retval = array(); foreach ( $res as $row ) { @@ -226,12 +227,16 @@ return $retval; } + public function clear() { + // nop, we dont store anything + } + /** * Only use from maintenance. Updates not implemented(yet), needs * handling for transactions. */ public function visitAll( $callback ) { - // storage is IteratorAggregate returning EchoBatchRowIterator of + // storage is commonly IteratorAggregate returning EchoBatchRowIterator of // 500 rows at a time by default. foreach ( $this->storage as $rows ) { foreach ( $rows as $row ) { @@ -268,7 +273,7 @@ protected function load( $row ) { $object = $this->mapper->fromStorageRow( $row ); foreach ( $this->lifecycleHandlers as $handler ) { - $handler->onPostLoad( $object, $row ); + $handler->onAfterLoad( $object, $row ); } return $object; } @@ -279,7 +284,7 @@ * Cant implement WritableObjectStorage because 'update' method signature differs */ class ObjectManager extends ObjectLocator { - // @var SplObjectHash $loaded If the object exists then an 'update' is issued, otherwise 'insert' + // @var SplObjectStorage $loaded If the object exists then an 'update' is issued, otherwise 'insert' protected $loaded; public function __construct( ObjectMapper $mapper, WritableObjectStorage $storage, array $indexes = array(), array $lifecycleHandlers = array() ) { @@ -314,7 +319,7 @@ $new = $this->mapper->toStorageRow( $object ); $this->storage->insert( $new ); foreach ( $this->lifecycleHandlers as $handler ) { - $handler->onPostInsert( $object, $new ); + $handler->onAfterInsert( $object, $new ); } $this->loaded[$object] = $new; } catch ( \Exception $e ) { @@ -331,7 +336,7 @@ } $this->storage->update( $old, $new ); foreach ( $this->lifecycleHandlers as $handler ) { - $handler->onPostUpdate( $object, $old, $new ); + $handler->onAfterUpdate( $object, $old, $new ); } $this->loaded[$object] = $new; } catch ( \Exception $e ) { @@ -340,17 +345,8 @@ } static public function arrayEquals( array $old, array $new ) { - foreach ( $old as $key => $value ) { - if ( !isset( $new[$key] ) || $new[$key] !== $value ) { - return false; - } - unset( $new[$key] ); - } - if ( !empty( $new ) ) { - return false; - } - // All keys in old match new and there are no left-over keys - return true; + return array_diff_assoc( $old, $new ) === array() + && array_diff_assoc( $new, $old ) === array(); } static public function makeArray( $input ) { @@ -366,7 +362,7 @@ $old = $this->loaded[$object]; $this->storage->remove( $old ); foreach ( $this->lifecycleHandlers as $handler ) { - $handler->onPostRemove( $object, $old ); + $handler->onAfterRemove( $object, $old ); } unset( $this->loaded[$object] ); } catch ( \Exception $e ) { @@ -375,7 +371,7 @@ } public function clear() { - $this->loaded = new SplObjectHash; + $this->loaded = new SplObjectStorage; } protected function load( $row ) { @@ -399,7 +395,6 @@ return null; } $split[$key] = $row[$key]; - unset( $row[$key] ); } return $split; @@ -419,7 +414,7 @@ /** * $userMapper = new BasicObjectMapper( * array( 'User', 'toStorageRow' ), - * array( 'User', 'newFromRow' ), + * array( 'User', 'fromStorageRow' ), * ); */ class BasicObjectMapper implements ObjectMapper { @@ -441,9 +436,9 @@ } // Doesn't support updating primary key value yet -class BasicDbStorage implements WritableObjectStorage, \IteratorAggregate { +class BasicDbStorage implements WritableObjectStorage { public function __construct( DbFactory $dbFactory, $table, array $primaryKey ) { - if ( empty( $primaryKey ) ) { + if ( !$primaryKey ) { throw new \Exception( 'PK required' ); } $this->dbFactory = $dbFactory; @@ -466,14 +461,14 @@ $missing = array_diff( $this->primaryKey, array_keys( $old ) ); throw new PersistenceException( 'Row has null primary key: ' . implode( $missing ) ); } + $updates = $this->calcUpdates( $old, $new ); + if ( !$updates ) { + return true; // nothing to change, success + } $dbw = $this->dbFactory->getDB( DB_MASTER ); - $res = $dbw->update( - $this->table, - $this->calcUpdates( $old, $new ), - UUID::convertUUIDs( $pk ), - __METHOD__ - ); // update returns boolean true/false as $res + $res = $dbw->update( $this->table, $updates, UUID::convertUUIDs( $pk ), __METHOD__ ); + // $dbw->update returns boolean true/false as $res // we also want to check that $pk actually selected a row to update return $res && $dbw->affectedRows(); } @@ -484,12 +479,11 @@ if ( !array_key_exists( $key, $old ) || $old[$key] !== $new[$key] ) { $updates[$key] = $new[$key]; } + unset( $old[$key] ); } - if ( !$updates ) { - echo '<pre>'; - var_dump( $old ); - var_dump( $new ); - die(); + // These keys dont exist in $new + foreach ( array_keys( $old ) as $key ) { + $updates[$key] = null; } return $updates; } @@ -571,18 +565,10 @@ } } -class NullLifecycleHandler implements LifecycleHandler { - function onPostInsert( $object, array $new ) { - } - function onPostUpdate( $object, array $old, array $new ) { - } - function onPostLoad( $object, array $old ) { - } - function onPostRemove( $object, array $old ) { - } -} - -abstract class AbstractIndex extends NullLifecycleHandler implements Index { +/** + * Index objects with similar features into the same buckets. + */ +abstract class FeatureIndex implements Index { abstract public function getLimit(); abstract public function queryOptions(); @@ -590,12 +576,18 @@ abstract protected function addToIndex( array $indexed, array $row ); abstract protected function removeFromIndex( array $indexed, array $row ); + /** + * @param BufferedCache $cache + * @param ObjectStorage $storage + * @param string $prefix + * @param array $indexedColumns List of columns to index, + */ public function __construct( BufferedCache $cache, ObjectStorage $storage, $prefix, array $indexedColumns ) { $this->cache = $cache; $this->storage = $storage; $this->prefix = $prefix; $this->indexed = $indexedColumns; - // sort this and ksort is self::cacheKey to always have cache key + // sort this and ksort in self::cacheKey to always have cache key // fields in same order sort( $indexedColumns ); $this->indexedOrdered = $indexedColumns; @@ -626,22 +618,37 @@ return true; } - public function onPostInsert( $object, array $new) { + public function onAfterInsert( $object, array $new) { $indexed = ObjectManager::splitFromRow( $new , $this->indexed ); - $this->addToIndex( $indexed, $this->compactRow( $new ) ); + // is un-indexable a bail-worthy occasion? Probably not + if ( $indexed ) { + $this->addToIndex( $indexed, $this->compactRow( $new ) ); + } } - public function onPostUpdate( $object, array $old, array $new ) { + public function onAfterUpdate( $object, array $old, array $new ) { $oldIndexed = ObjectManager::splitFromRow( $old, $this->indexed ); $newIndexed = ObjectManager::splitFromRow( $new, $this->indexed ); // TODO: optimize $oldIndexed === $newIndexed, which should be common - $this->removeFromIndex( $oldIndexed, $this->compactRow( $old ) ); - $this->addToIndex( $newIndexed, $this->compactRow( $new ) ); + // TODO: optimize out occurances where data changed but not the values + // indexed or stored in the index(for shallow indexes) + if ( $oldIndexed ) { + $this->removeFromIndex( $oldIndexed, $this->compactRow( $old ) ); + } + if ( $newIndexed ) { + $this->addToIndex( $newIndexed, $this->compactRow( $new ) ); + } } - public function onPostRemove( $object, array $old ) { + public function onAfterRemove( $object, array $old ) { $indexed = ObjectManager::splitFromRow( $old, $this->indexed ); - $this->removeFromIndex( $indexed, $this->compactRow( $old ) ); + if ( $indexed ) { + $this->removeFromIndex( $indexed, $this->compactRow( $old ) ); + } + } + + public function onAfterLoad( $object, array $old ) { + // nothing to do } public function find( array $attributes ) { @@ -664,11 +671,9 @@ ); } $key = $this->cacheKey( $query ); + // allow for duplicate queries $keyToIdx[$key][] = $idx; - if ( isset( $keyToQuery[$key] ) ) { - // duplicate query - unset( $queries[$idx] ); - } else { + if ( !isset( $keyToQuery[$key] ) ) { $idxToKey[$idx] = $key; $keyToQuery[$key] = $query; } @@ -709,6 +714,13 @@ protected function cacheKey( array $attributes ) { + global $wgFlowDefaultWikiDb; // meh + if ( $wgFlowDefaultWikiDb === false ) { + $db = wfWikiId(); + } else { + $db = $wgFlowDefaultWikiDb; + } + foreach( $attributes as $key => $attr ) { if ( $attr instanceof \Flow\Model\UUID ) { $attributes[$key] = $attr->getHex(); @@ -718,7 +730,7 @@ } } - return wfForeignMemcKey( 'flow', '', $this->prefix, implode( ':', $attributes ) ); + return wfForeignMemcKey( $db, '', $this->prefix, implode( ':', $attributes ) ); } /** @@ -755,7 +767,11 @@ } } -class UniqueIndex extends AbstractIndex { +/** + * Offers direct lookup of an object via a unique feature(set of properties) + * on the object. + */ +class UniqueFeatureIndex extends FeatureIndex { public function getLimit() { return 1; @@ -781,13 +797,16 @@ } } -class SecondaryIndex extends AbstractIndex { +/** + * Holds the top k items matching + */ +class TopKIndex extends FeatureIndex { public function __construct( BufferedCache $cache, ObjectStorage $storage, $prefix, array $indexed, array $options = array() ) { if ( empty( $options['sort'] ) ) { - throw new \InvalidArgumentException( 'SecondaryIndex must be sorted' ); + throw new \InvalidArgumentException( 'TopKIndex must be sorted' ); } - if ( isset( $options['shallow'] ) && !$options['shallow'] instanceof UniqueIndex ) { - throw new \InvalidArgumentException( "The 'shallow' option must be either null or UniqueIndex." ); + if ( isset( $options['shallow'] ) && !$options['shallow'] instanceof UniqueFeatureIndex ) { + throw new \InvalidArgumentException( "The 'shallow' option must be either null or UniqueFeatureIndex." ); } parent::__construct( $cache, $storage, $prefix, $indexed ); @@ -896,16 +915,13 @@ protected function expandCacheResult( array $cached, array $keyToQuery ) { $cached = parent::expandCacheResult( $cached, $keyToQuery ); - if ( $this->options['shallow'] ) { + if ( $cached && $this->options['shallow'] ) { return $this->expandShallowResult( $cached ); } return $cached; } protected function expandShallowResult( array $results ) { - if ( !$results ) { - return array(); - } // Allows us to flatten $results into a single $query array, then // rebuild final return value in same structure and order as $results. $duplicator = new ResultDuplicator( $this->options['shallow']->getPrimaryKeyColumns(), 2 ); @@ -1050,6 +1066,7 @@ public function getMulti( array $keys ) { return $this->cache->getMulti( $keys ); } + public function add( $key, $value, $exptime = 0 ) { if ( $this->buffer === null ) { $this->cache->add( $key, $value, $exptime ); diff --git a/includes/Data/RevisionStorage.php b/includes/Data/RevisionStorage.php index b954286..22a65fb 100644 --- a/includes/Data/RevisionStorage.php +++ b/includes/Data/RevisionStorage.php @@ -12,6 +12,7 @@ abstract class RevisionStorage implements WritableObjectStorage { static protected $allowedUpdateColumns = array( 'rev_flags' ); protected $dbFactory; + protected $externalStores; abstract protected function joinTable(); abstract protected function relatedPk(); @@ -21,19 +22,18 @@ abstract protected function updateRelated( array $rev, array $related ); abstract protected function removeRelated( array $row ); - public function __construct( DbFactory $dbFactory ) { + public function __construct( DbFactory $dbFactory, $externalStore ) { $this->dbFactory = $dbFactory; + $this->externalStore = $externalStore; } // Find one by specific attributes public function find( array $attributes, array $options = array() ) { - global $wgFlowExternalStore; - $res = $this->findInternal( $attributes, $options ); if ( $res === false ) { return false; } - if ( $wgFlowExternalStore ) { + if ( $this->externalStore ) { $res = Merger::merge( $res, 'text_content', @@ -67,14 +67,12 @@ } public function findMulti( array $queries, array $options = array() ) { - global $wgFlowExternalStore; - if ( count( $queries ) < 3 ) { $res = $this->fallbackFindMulti( $queries, $options ); } else { $res = $this->findMultiInternal( $queries, $options ); } - if ( $wgFlowExternalStore ) { + if ( $this->externalStore ) { return Merger::mergeMulti( $res, 'text_content', @@ -234,12 +232,11 @@ } protected function insertContent( $data ) { - global $wgFlowExternalStore; $flags = \Revision::compressRevisionText( $data ); - if ( $wgFlowExternalStore ) { + if ( $this-externalStore ) { // Store and get the URL - $data = ExternalStore::insertWithFallback( $data, array(), $wgFlowExternalStore ); + $data = ExternalStore::insertWithFallback( $this->externalStore, $data ); if ( !$data ) { throw new \MWException( "Unable to store text to external storage" ); } @@ -312,6 +309,10 @@ return array( 'rev_id' ); } + public function getIterator() { + throw new \MWException( 'Not Implemented' ); + } + // Separtes $row into two arrays, one with the rev_ prefix // and the other with everything else. May need to split more // specifically if we want > 2 prefixes. @@ -331,8 +332,8 @@ class PostRevisionStorage extends RevisionStorage { - public function __construct( DbFactory $dbFactory, TreeRepository $treeRepo ) { - parent::__construct( $dbFactory ); + public function __construct( DbFactory $dbFactory, $externalStore, TreeRepository $treeRepo ) { + parent::__construct( $dbFactory, $externalStore ); $this->treeRepo = $treeRepo; } @@ -428,7 +429,7 @@ } /** - * This assists in performing client-side joins. It collects the foreign key + * This assists in performing client-side 1-to-1 joins. It collects the foreign key * from a multi-dimensional array, queries a callable for the foreign key values and * then returns the source data with related data merged in. */ diff --git a/includes/Model/AbstractRevision.php b/includes/Model/AbstractRevision.php index bc84d78..335f303 100644 --- a/includes/Model/AbstractRevision.php +++ b/includes/Model/AbstractRevision.php @@ -10,6 +10,8 @@ protected $userId; protected $userText; protected $flags = array(); + // An i18n message key indicating what kind of change this revision is + // primary use case is the a revision history list. protected $comment; protected $prevRevision; @@ -20,6 +22,13 @@ static public function fromStorageRow( array $row ) { $obj = new static; + if ( $row['rev_type'] !== $obj->getRevisionType() ) { + throw new \MWException( sprintf( + "Wrong revision type, expected '%s' but received '%s'", + $obj->getRevisionType(), + $row['rev_type'] + ) ); + } $obj->revId = UUID::create( $row['rev_id'] ); $obj->userId = $row['rev_user_id']; $obj->userText = $row['rev_user_text']; @@ -48,8 +57,8 @@ 'rev_flags' => implode( ',', $obj->flags ), 'rev_parent_id' => $prevRevision, 'rev_comment' => $obj->comment, - 'rev_text_id' => $obj->textId, + 'rev_type' => $obj->getRevisionType(), 'text_content' => $obj->content, ); @@ -87,11 +96,6 @@ return $this->content; } - // internal: for lazy loading from external source - public function setContent( $content ) { - $this->content = $content; - } - public function getTextId() { return $this->textId; // not available on creation } @@ -109,6 +113,9 @@ // already flagged return $this; } + if ( false !== strpos( ',', $flag ) ) { + throw new \MWException( 'Invalid flag name: contains comma' ); + } $updated = $this->newNullRevision( $user ); $updated->flags[] = $flag; $updated->comment = $comment; diff --git a/includes/Model/PostRevision.php b/includes/Model/PostRevision.php index 81eb5ac..3be1956 100644 --- a/includes/Model/PostRevision.php +++ b/includes/Model/PostRevision.php @@ -30,24 +30,17 @@ $obj->origCreateTime = wfTimestampNow(); $obj->replyToId = null; // not a reply to anything $obj->prevRevId = null; // no parent revision + $obj->comment = 'flow-rev-message-new-post'; return $obj; } static public function fromStorageRow( array $row ) { - if ( $row['rev_type'] !== 'post' ) { - throw new \MWException( "Wrong revision type, expected 'post' but got : " . $row['rev_type'] ); - } if ( $row['rev_id'] !== $row['tree_rev_id'] ) { throw new \MWException( 'tree revision doesn\'t match provided revision' ); } $obj = parent::fromStorageRow( $row ); - if ( $row['tree_parent_id'] ) { - $obj->replyToId = UUID::create( $row['tree_parent_id'] ); - } else { - $obj->replyToId = null; - } - + $obj->replyToId = UUID::create( $row['tree_parent_id'] ); $obj->postId = UUID::create( $row['tree_rev_descendant'] ); $obj->origCreateTime = $row['tree_orig_create_time']; $obj->origUserId = $row['tree_orig_user_id']; @@ -57,14 +50,8 @@ } static public function toStorageRow( $rev ) { - $replyToId = null; - - if ( $rev->replyToId ) { - $replyToId = $rev->replyToId->getBinary(); - } return parent::toStorageRow( $rev ) + array( - 'rev_type' => 'post', - 'tree_parent_id' => $replyToId, + 'tree_parent_id' => $rev->replyToId ? $rev->replyToId->getBinary() : null, 'tree_rev_descendant' => $rev->postId->getBinary(), 'tree_rev_id' => $rev->revId->getBinary(), // rest of tree_ is denormalized data about first post revision @@ -83,6 +70,7 @@ $reply->origCreateTime = wfTimestampNow(); $reply->content = $content; $reply->replyToId = $this->postId; + $reply->comment = 'flow-rev-message-reply'; return $reply; } @@ -125,6 +113,10 @@ public function compareCreateTime( PostRevision $rev ) { return strcmp( $rev->postId->getNumber(), $this->postId->getNumber() ); } + + public function getRevisionType() { + return 'post'; + } } diff --git a/includes/Model/Summary.php b/includes/Model/Summary.php index 4fc8250..238a874 100644 --- a/includes/Model/Summary.php +++ b/includes/Model/Summary.php @@ -19,9 +19,6 @@ } static public function fromStorageRow( array $row ) { - if ( $row['rev_type'] !== 'summary' ) { - throw new \MWException( 'Wrong revision type: ' . $row['rev_type'] ); - } $obj = parent::fromStorageRow( $row ); $obj->workflowId = UUID::create( $row['summary_workflow_id'] ); return $obj; @@ -34,5 +31,9 @@ 'summary_workflow_id' => $obj->workflowId->getBinary(), ); } + + public function getRevisionType() { + return 'summary'; + } } diff --git a/includes/Model/Workflow.php b/includes/Model/Workflow.php index 180663c..1060dd5 100644 --- a/includes/Model/Workflow.php +++ b/includes/Model/Workflow.php @@ -2,6 +2,7 @@ namespace Flow\Model; +use SpecialPage; use Title; use User; @@ -83,7 +84,7 @@ public function getTitle() { $article = $this->getArticleTitle(); - return Title::newFromText( "Flow/" . $article->getPrefixedDBkey(), NS_SPECIAL ); + return SpecialPage::getTitleFor( 'Flow', $article->getPrefixedDBkey() ); } public function getArticleTitle() { @@ -115,12 +116,14 @@ } } + // these are exceptions currently to make debugging easier + // it should return false later on to allow wider use. public function matchesTitle( Title $title ) { if ( $title->getArticleID() === 0 ) { return false; // non-existant page } if ( $title->getNamespace() !== $this->namespace ) { - var_dump( $title->getNamespace()); + var_dump( $title->getNamespace() ); var_dump( $this->namespace ); die(); throw new \Exception( 'namespace' ); diff --git a/includes/Repository/SelectQueryBuilder.php b/includes/Repository/SelectQueryBuilder.php index bfed3ed..be1be98 100644 --- a/includes/Repository/SelectQueryBuilder.php +++ b/includes/Repository/SelectQueryBuilder.php @@ -16,10 +16,6 @@ return new self; } - public function setCacheKey( $cacheKey ) { - $this->cacheKey = $cacheKey; - } - public function from( $table ) { $this->table = $table; return $this; @@ -69,10 +65,10 @@ list( $field, $op, $value ) = $row; $conditions[] = "$field $op " . $dbr->addQuotes( $value ); } - $row = $db->select( $this->table, $this->fields, $conditions, $fname, $this->options ); + $res = $db->select( $this->table, $this->fields, $conditions, $fname, $this->options ); if ( $this->resultHandler ) { - $row = call_user_func( $this->resultHandler, $row ); + return call_user_func( $this->resultHandler, $res ); } - return $row; + return $res; } } diff --git a/includes/Templating.php b/includes/Templating.php index 3ef4eaf..66243ec 100644 --- a/includes/Templating.php +++ b/includes/Templating.php @@ -19,6 +19,10 @@ $this->globals = $globals; } + public function getOutput() { + return $this->output; + } + public function addNamespace( $ns, $path ) { $this->namespaces[$ns] = rtrim( $path, '/' ); } diff --git a/includes/UrlGenerator.php b/includes/UrlGenerator.php index 6d62061..72fdd81 100644 --- a/includes/UrlGenerator.php +++ b/includes/UrlGenerator.php @@ -14,6 +14,7 @@ public function generateUrl( $workflow, $action = 'view', array $query = array() ) { if ( ! $workflow instanceof Workflow ) { $workflowId = $workflow; + // Only way to know what title the workflow points at $workflow = $this->storage->get( $workflowId ); if ( !$workflow ) { throw new \MWException( "Unknown flow object: $workflowId" ); diff --git a/modules/base/ext.flow.base.js b/modules/base/ext.flow.base.js new file mode 100644 index 0000000..02405dd --- /dev/null +++ b/modules/base/ext.flow.base.js @@ -0,0 +1 @@ +alert( 'foo' ); diff --git a/special/SpecialFlow.php b/special/SpecialFlow.php index 1272270..47fba3c 100644 --- a/special/SpecialFlow.php +++ b/special/SpecialFlow.php @@ -91,8 +91,7 @@ ) ); if ( $found ) { $definition = reset( $found ); - } - if ( empty( $definition ) ) { + } else { throw new MWException( "Unknown flow type '$workflowName' requested" ); } } diff --git a/vendor/Pimple.php b/vendor/Pimple.php index 7537cfa..39f8daf 100644 --- a/vendor/Pimple.php +++ b/vendor/Pimple.php @@ -25,7 +25,7 @@ */ /** - * Pimple main class. + * Pimple main class. http://pimple.sensiolabs.org/ * * @package pimple * @author Fabien Potencier -- To view, visit https://gerrit.wikimedia.org/r/78186 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: newchange Gerrit-Change-Id: If85dbc1a9c8558e4ec0f5b8c2030f3e08b5dc399 Gerrit-PatchSet: 1 Gerrit-Project: mediawiki/extensions/Flow Gerrit-Branch: master Gerrit-Owner: EBernhardson (WMF) <ebernhard...@wikimedia.org> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits