jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/314480 )
Change subject: Improve error-handling for OptInController
......................................................................
Improve error-handling for OptInController
* Better logging for failures (before it just logged
the exception, now we also say what happened)
* Rollback all databases on failure.
* Use MWCallableUpdate. Not sure if this is required,
but it will make sure the callback is not run if the
database in question rolls back before the callback starts.
* Also, use dependency injection.
Bug: T138310
Change-Id: I074586aa1a718bfb6905e33ce999ad8832fb6aaa
---
M Hooks.php
M container.php
M extension.json
M includes/Import/OptInController.php
D includes/Import/OptInUpdate.php
5 files changed, 111 insertions(+), 70 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/Hooks.php b/Hooks.php
index de887c1..052fe03 100644
--- a/Hooks.php
+++ b/Hooks.php
@@ -8,7 +8,7 @@
use Flow\Exception\PermissionException;
use Flow\Data\Listener\RecentChangesListener;
use Flow\Formatter\CheckUserQuery;
-use Flow\Import\OptInUpdate;
+use Flow\Import\OptInController;
use Flow\Model\UUID;
use Flow\OccupationController;
use Flow\SpamFilter\AbuseFilter;
@@ -1754,20 +1754,20 @@
$after = $user->getBoolOption( BETA_FEATURE_FLOW_USER_TALK_PAGE
);
$action = null;
+ $optInController = Flow\Container::get( 'controller.opt_in' );
if ( !$before && $after ) {
- $action = OptInUpdate::$ENABLE;
+ $action = OptInController::$ENABLE;
// Check if the user had a flow board
- $c = new Flow\Import\OptInController();
- if ( !$c->hasFlowBoardArchive( $user ) ) {
+ if ( !$optInController->hasFlowBoardArchive( $user ) ) {
// Enable the guided tour by setting the cookie
RequestContext::getMain()->getRequest()->response()->setCookie(
'Flow_optIn_guidedTour', '1' );
}
} elseif ( $before && !$after ) {
- $action = OptInUpdate::$DISABLE;
+ $action = OptInController::$DISABLE;
}
if ( $action ) {
- DeferredUpdates::addUpdate( new OptInUpdate( $action,
$user->getTalkPage(), $user ) );
+ $optInController->initiateChange( $action,
$user->getTalkPage(), $user );
}
return true;
diff --git a/container.php b/container.php
index e7ab7df..ff79ba9 100644
--- a/container.php
+++ b/container.php
@@ -781,6 +781,22 @@
// must always happen before calling flow code.
$c['occupation_controller'] = FlowHooks::getOccupationController();
+$c['helper.archive_name'] = function ( $c ) {
+ return new Flow\Import\ArchiveNameHelper();
+};
+
+$c['controller.opt_in'] = function ( $c ) {
+ return new Flow\Import\OptInController(
+ $c['occupation_controller'],
+ $c['controller.notification'],
+ $c['helper.archive_name'],
+ $c['db.factory'],
+ $c['default_logger'],
+ $c['occupation_controller']->getTalkpageManager()
+
+ );
+};
+
$c['controller.notification'] = function ( $c ) {
global $wgContLang;
return new Flow\NotificationController( $wgContLang,
$c['repository.tree'] );
diff --git a/extension.json b/extension.json
index ce0ab2d..e766273 100644
--- a/extension.json
+++ b/extension.json
@@ -1109,7 +1109,6 @@
"Flow\\Import\\LiquidThreadsApi\\ScriptedImportRevision":
"includes/Import/LiquidThreadsApi/Objects.php",
"Flow\\Import\\LiquidThreadsApi\\TopicIterator":
"includes/Import/LiquidThreadsApi/Iterators.php",
"Flow\\Import\\OptInController":
"includes/Import/OptInController.php",
- "Flow\\Import\\OptInUpdate": "includes/Import/OptInUpdate.php",
"Flow\\Import\\PageImportState": "includes/Import/Importer.php",
"Flow\\Import\\Plain\\ImportHeader":
"includes/Import/Plain/ImportHeader.php",
"Flow\\Import\\Plain\\ObjectRevision":
"includes/Import/Plain/ObjectRevision.php",
diff --git a/includes/Import/OptInController.php
b/includes/Import/OptInController.php
index 7fd1171..49d9e84 100644
--- a/includes/Import/OptInController.php
+++ b/includes/Import/OptInController.php
@@ -4,7 +4,10 @@
use DateTime;
use DateTimeZone;
+use DeferredUpdates;
use DerivativeContext;
+use Exception;
+use Flow\DbFactory;
use Flow\Collection\HeaderCollection;
use Flow\Content\BoardContent;
use Flow\Exception\InvalidDataException;
@@ -14,7 +17,9 @@
use Flow\WorkflowLoader;
use Flow\WorkflowLoaderFactory;
use IContextSource;
+use Psr\Log\LoggerInterface;
use MovePage;
+use MWExceptionHandler;
use Parser;
use ParserOptions;
use RequestContext;
@@ -29,6 +34,8 @@
* Entry point for enabling Flow on a page.
*/
class OptInController {
+ public static $ENABLE = 'enable';
+ public static $DISABLE = 'disable';
/**
* @var OccupationController
@@ -46,6 +53,16 @@
private $archiveNameHelper;
/**
+ * @var DbFactory
+ */
+ private $dbFactory;
+
+ /**
+ * @var LoggerInterface
+ */
+ private $logger;
+
+ /**
* @var IContextSource
*/
private $context;
@@ -55,16 +72,83 @@
*/
private $user;
- public function __construct() {
- $this->occupationController = Container::get(
'occupation_controller' );
- $this->notificationController = Container::get(
'controller.notification' );
- $this->archiveNameHelper = new ArchiveNameHelper();
- $this->user = $this->occupationController->getTalkpageManager();
+ /**
+ * @param OccupationController $occupationController
+ * @param NotificationController $notificationController
+ * @param ArchiveNameHelper $archiveNameHelper
+ * @param DbFactory $dbFactory
+ * @param LoggerInterface $logger Logger for errors and exceptions
+ * @param User $scriptUser User that takes actions, such as creating
the board or
+ * editing descriptions
+ */
+ public function __construct(
+ OccupationController $occupationController,
+ NotificationController $notificationController,
+ ArchiveNameHelper $archiveNameHelper,
+ DbFactory $dbFactory,
+ LoggerInterface $logger,
+ User $scriptUser
+ ) {
+ $this->occupationController = $occupationController;
+ $this->notificationController = $notificationController;
+ $this->archiveNameHelper = $archiveNameHelper;
+ $this->dbFactory = $dbFactory;
+ $this->logger = $logger;
+ $this->user = $scriptUser;
$this->context = new DerivativeContext(
RequestContext::getMain() );
$this->context->setUser( $this->user );
}
/**
+ * @param string $action Action to take, self::ENABLE or self::DISABLE
+ * @param Title $talkpage Title of user's talk page
+ * @param User $user User that owns the talk page
+ */
+ public function initiateChange( $action, Title $talkpage, User $user ) {
+ $flowDbw = $this->dbFactory->getDB( DB_MASTER );
+ $wikiDbw = $this->dbFactory->getWikiDB( DB_MASTER );
+
+ $outerMethod = __METHOD__;
+ $logger = $this->logger;
+
+ // We need both since we use both databases.
+ DeferredUpdates::addCallableUpdate(
+ function () use ( $logger, $outerMethod, $wikiDbw,
$action, $talkpage, $user ) {
+ DeferredUpdates::addCallableUpdate(
+ function () use ( $logger,
$outerMethod, $action, $talkpage, $user ) {
+ try {
+ if ( $action ===
self::$ENABLE ) {
+ $this->enable(
$talkpage, $user );
+ } elseif ( $action ===
self::$DISABLE ) {
+ $this->disable(
$talkpage );
+ } else {
+ $logger->error(
$outerMethod . ': unrecognized action: ' . $action );
+ }
+ } catch ( Exception $e ) {
+ $logger->error(
+ $outerMethod .
' failed to {action} Flow on \'{talkpage}\' for user \'{user}\'. Exception:
{exception}',
+ [
+
'action' => $action,
+
'talkpage' => $talkpage,
+ 'user'
=> $user,
+
'exception' => $e,
+ ]
+ );
+ // rollback both Flow
and Core DBs
+
MWExceptionHandler::rollbackMasterChangesAndLog( $e );
+
$this->dbFactory->getDB( DB_MASTER )->rollback( $outerMethod );
+ }
+ },
+ $outerMethod,
+ $wikiDbw
+ );
+ },
+ __METHOD__,
+ $flowDbw
+ );
+ }
+
+ /**
* @param Title $title
* @param User $user
*/
diff --git a/includes/Import/OptInUpdate.php b/includes/Import/OptInUpdate.php
deleted file mode 100644
index 3c1a0f7..0000000
--- a/includes/Import/OptInUpdate.php
+++ /dev/null
@@ -1,58 +0,0 @@
-<?php
-
-namespace Flow\Import;
-
-use DeferrableUpdate;
-use MWExceptionHandler;
-use Title;
-use User;
-
-class OptInUpdate implements DeferrableUpdate {
-
- public static $ENABLE = 'enable';
- public static $DISABLE = 'disable';
-
- /**
- * @var string
- */
- protected $action;
-
- /**
- * @var Title
- */
- protected $talkpage;
-
- /**
- * @var User
- */
- protected $user;
-
- /**
- * @param string $action
- * @param Title $talkpage
- * @param User $user
- */
- public function __construct( $action, Title $talkpage, User $user ) {
- $this->action = $action;
- $this->talkpage = $talkpage;
- $this->user = $user;
- }
-
- /**
- * Enable or disable Flow on a talk page
- */
- function doUpdate() {
- $c = new OptInController();
- try {
- if ( $this->action === self::$ENABLE ) {
- $c->enable( $this->talkpage, $this->user );
- } elseif ( $this->action === self::$DISABLE ) {
- $c->disable( $this->talkpage );
- } else {
- wfLogWarning( 'OptInUpdate: unrecognized
action: ' . $this->action );
- }
- } catch ( \Exception $e ) {
- MWExceptionHandler::logException( $e );
- }
- }
-}
--
To view, visit https://gerrit.wikimedia.org/r/314480
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I074586aa1a718bfb6905e33ce999ad8832fb6aaa
Gerrit-PatchSet: 5
Gerrit-Project: mediawiki/extensions/Flow
Gerrit-Branch: master
Gerrit-Owner: Mattflaschen <[email protected]>
Gerrit-Reviewer: Aaron Schulz <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Sbisson <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
