jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/392411 )
Change subject: More logging
......................................................................
More logging
This adds lots of logging in various places of various levels
for the FileImporter channel.
It also switched from the optional logger interfaces to requiring
one in the constructor for many services.
Each individual import operation now has logging, although
logging within the import done in the WikiRevision class will
go to a different log channel..
It would be great to be able to inject our own log channel here?
or somehow get the logs out for inclusion in our own log channel.
Bug: T180491
Change-Id: I9c5f6d977c62b17c3ef3d92d0d4580e2c1e9fbb3
---
M src/Data/FileRevision.php
M src/Data/ImportOperations.php
M src/Data/TextRevision.php
M src/Operations/FileRevisionFromRemoteUrl.php
M src/Operations/TextRevisionFromTextRevision.php
M src/Remote/MediaWiki/ApiDetailRetriever.php
M src/Remote/MediaWiki/RemoteApiImportTitleChecker.php
M src/Remote/MediaWiki/SiteTableSourceUrlChecker.php
M src/ServiceWiring.php
M src/Services/Importer.php
M src/SpecialImportFile.php
M tests/phpunit/MediaWiki/SiteTableSourceUrlCheckerTest.php
M tests/phpunit/Remote/MediaWiki/ApiDetailRetrieverTest.php
13 files changed, 170 insertions(+), 63 deletions(-)
Approvals:
jenkins-bot: Verified
Andrew-WMDE: Looks good to me, approved
diff --git a/src/Data/FileRevision.php b/src/Data/FileRevision.php
index 098e02e..a53c83c 100644
--- a/src/Data/FileRevision.php
+++ b/src/Data/FileRevision.php
@@ -59,4 +59,11 @@
return $this->fields[$name];
}
+ /**
+ * @return array
+ */
+ public function getFields() {
+ return $this->fields;
+ }
+
}
diff --git a/src/Data/ImportOperations.php b/src/Data/ImportOperations.php
index 0a5e988..cf3029c 100644
--- a/src/Data/ImportOperations.php
+++ b/src/Data/ImportOperations.php
@@ -45,7 +45,6 @@
$this->state = self::PREPARE_RUN;
foreach ( $this->importOperations as $importOperation ) {
if ( !$importOperation->prepare() ) {
- // TODO log?
return false;
}
}
@@ -61,7 +60,6 @@
$this->state = self::COMMIT_RUN;
foreach ( $this->importOperations as $importOperation ) {
if ( !$importOperation->commit() ) {
- // TODO log?
return false;
}
}
diff --git a/src/Data/TextRevision.php b/src/Data/TextRevision.php
index 899bab8..ddb5a69 100644
--- a/src/Data/TextRevision.php
+++ b/src/Data/TextRevision.php
@@ -60,4 +60,11 @@
return $this->fields[$name];
}
+ /**
+ * @return array
+ */
+ public function getFields() {
+ return $this->fields;
+ }
+
}
diff --git a/src/Operations/FileRevisionFromRemoteUrl.php
b/src/Operations/FileRevisionFromRemoteUrl.php
index 24ad537..96e5e75 100644
--- a/src/Operations/FileRevisionFromRemoteUrl.php
+++ b/src/Operations/FileRevisionFromRemoteUrl.php
@@ -8,6 +8,7 @@
use FileImporter\Services\UploadBase\UploadBaseFactory;
use FileImporter\Services\WikiRevisionFactory;
use Http;
+use Psr\Log\LoggerInterface;
use TempFSFile;
use Title;
use WikiRevision;
@@ -40,6 +41,11 @@
private $uploadBaseFactory;
/**
+ * @var LoggerInterface
+ */
+ private $logger;
+
+ /**
* @var WikiRevision|null
*/
private $wikiRevision;
@@ -49,13 +55,15 @@
FileRevision $fileRevision,
HttpRequestExecutor $httpRequestExecutor,
WikiRevisionFactory $wikiRevisionFactory,
- UploadBaseFactory $uploadBaseFactory
+ UploadBaseFactory $uploadBaseFactory,
+ LoggerInterface $logger
) {
$this->plannedTitle = $plannedTitle;
$this->fileRevision = $fileRevision;
$this->httpRequestExecutor = $httpRequestExecutor;
$this->wikiRevisionFactory = $wikiRevisionFactory;
$this->uploadBaseFactory = $uploadBaseFactory;
+ $this->logger = $logger;
}
/**
@@ -87,7 +95,17 @@
$this->plannedTitle,
$this->wikiRevision->getFileSrc()
);
- return $base->validateTitle() === true && $base->validateFile()
=== true;
+
+ $result = $base->validateTitle() === true &&
$base->validateFile() === true;
+
+ if ( !$result ) {
+ $this->logger->error(
+ __METHOD__ . ' failed to prepare.',
+ $this->fileRevision->getFields()
+ );
+ }
+
+ return $result;
}
/**
@@ -95,7 +113,16 @@
* @return bool success
*/
public function commit() {
- return $this->wikiRevision->importUpload();
+ $result = $this->wikiRevision->importUpload();
+
+ if ( !$result ) {
+ $this->logger->error(
+ __METHOD__ . ' failed to commit.',
+ $this->fileRevision->getFields()
+ );
+ }
+
+ return $result;
}
}
diff --git a/src/Operations/TextRevisionFromTextRevision.php
b/src/Operations/TextRevisionFromTextRevision.php
index e1b10b6..8b28862 100644
--- a/src/Operations/TextRevisionFromTextRevision.php
+++ b/src/Operations/TextRevisionFromTextRevision.php
@@ -5,6 +5,7 @@
use FileImporter\Data\TextRevision;
use FileImporter\Interfaces\ImportOperation;
use FileImporter\Services\WikiRevisionFactory;
+use Psr\Log\LoggerInterface;
use Title;
use WikiRevision;
@@ -26,6 +27,11 @@
private $wikiRevisionFactory;
/**
+ * @var LoggerInterface
+ */
+ private $logger;
+
+ /**
* @var WikiRevision|null
*/
private $wikiRevision;
@@ -33,11 +39,13 @@
public function __construct(
Title $plannedTitle,
TextRevision $textRevision,
- WikiRevisionFactory $wikiRevisionFactory
+ WikiRevisionFactory $wikiRevisionFactory,
+ LoggerInterface $logger
) {
$this->plannedTitle = $plannedTitle;
$this->textRevision = $textRevision;
$this->wikiRevisionFactory = $wikiRevisionFactory;
+ $this->logger = $logger;
}
/**
@@ -57,7 +65,16 @@
* @return bool success
*/
public function commit() {
- return $this->wikiRevision->importOldRevision();
+ $result = $this->wikiRevision->importOldRevision();
+
+ if ( !$result ) {
+ $this->logger->error(
+ __METHOD__ . ' failed to commit.',
+ $this->textRevision->getFields()
+ );
+ }
+
+ return $result;
}
}
diff --git a/src/Remote/MediaWiki/ApiDetailRetriever.php
b/src/Remote/MediaWiki/ApiDetailRetriever.php
index 5286b5f..9ed5ebf 100644
--- a/src/Remote/MediaWiki/ApiDetailRetriever.php
+++ b/src/Remote/MediaWiki/ApiDetailRetriever.php
@@ -14,12 +14,10 @@
use FileImporter\Interfaces\DetailRetriever;
use FileImporter\Services\Http\HttpRequestExecutor;
use Message;
-use Psr\Log\LoggerAwareInterface;
use Psr\Log\LoggerInterface;
-use Psr\Log\NullLogger;
use Title;
-class ApiDetailRetriever implements DetailRetriever, LoggerAwareInterface {
+class ApiDetailRetriever implements DetailRetriever {
const MAX_REVISIONS = 1000;
@@ -40,14 +38,11 @@
public function __construct(
HttpApiLookup $httpApiLookup,
- HttpRequestExecutor $httpRequestExecutor
+ HttpRequestExecutor $httpRequestExecutor,
+ LoggerInterface $logger
) {
$this->httpApiLookup = $httpApiLookup;
$this->httpRequestExecutor = $httpRequestExecutor;
- $this->logger = new NullLogger();
- }
-
- public function setLogger( LoggerInterface $logger ) {
$this->logger = $logger;
}
diff --git a/src/Remote/MediaWiki/RemoteApiImportTitleChecker.php
b/src/Remote/MediaWiki/RemoteApiImportTitleChecker.php
index 9a6a047..d381d9d 100644
--- a/src/Remote/MediaWiki/RemoteApiImportTitleChecker.php
+++ b/src/Remote/MediaWiki/RemoteApiImportTitleChecker.php
@@ -7,18 +7,22 @@
use FileImporter\Exceptions\ImportException;
use FileImporter\Interfaces\ImportTitleChecker;
use FileImporter\Services\Http\HttpRequestExecutor;
+use Psr\Log\LoggerInterface;
class RemoteApiImportTitleChecker implements ImportTitleChecker {
private $httpApiLookup;
private $httpRequestExecutor;
+ private $logger;
public function __construct(
HttpApiLookup $httpApiLookup,
- HttpRequestExecutor $httpRequestExecutor
+ HttpRequestExecutor $httpRequestExecutor,
+ LoggerInterface $logger
) {
$this->httpApiLookup = $httpApiLookup;
$this->httpRequestExecutor = $httpRequestExecutor;
+ $this->logger = $logger;
}
/**
@@ -35,11 +39,24 @@
try {
$imageInfoRequest =
$this->httpRequestExecutor->execute( $requestUrl );
} catch ( HttpRequestException $e ) {
+ $this->logger->error(
+ __METHOD__ . ' failed to check title state
from: ' . $requestUrl,
+ [
+ 'url' =>
$e->getHttpRequest()->getFinalUrl(),
+ 'content' =>
$e->getHttpRequest()->getContent(),
+ 'errors' =>
$e->getStatusValue()->getErrors(),
+ ]
+ );
throw new ImportException( 'Failed to check title state
from: ' . $requestUrl );
}
$requestData = json_decode( $imageInfoRequest->getContent(),
true );
- return array_key_exists( '-1', $requestData['query']['pages'] );
+ $result = array_key_exists( '-1',
$requestData['query']['pages'] );
+ if ( !$result ) {
+ $this->logger->error( __METHOD__ . ' failed, could not
find pages query key in result.' );
+ }
+
+ return $result;
}
private function getParams( $titleString ) {
diff --git a/src/Remote/MediaWiki/SiteTableSourceUrlChecker.php
b/src/Remote/MediaWiki/SiteTableSourceUrlChecker.php
index c755319..72795ef 100644
--- a/src/Remote/MediaWiki/SiteTableSourceUrlChecker.php
+++ b/src/Remote/MediaWiki/SiteTableSourceUrlChecker.php
@@ -4,6 +4,7 @@
use FileImporter\Data\SourceUrl;
use FileImporter\Interfaces\SourceUrlChecker;
+use Psr\Log\LoggerInterface;
/**
* This SourceUrlChecker implementation will allow files from mediawiki
websites that are contained
@@ -13,15 +14,35 @@
private $siteTableSiteLookup;
+ /**
+ * @var LoggerInterface
+ */
+ private $logger;
+
public function __construct(
- SiteTableSiteLookup $siteTableSiteLookup
+ SiteTableSiteLookup $siteTableSiteLookup,
+ LoggerInterface $logger
) {
$this->siteTableSiteLookup = $siteTableSiteLookup;
+ $this->logger = $logger;
}
public function checkSourceUrl( SourceUrl $sourceUrl ) {
- return $this->siteTableSiteLookup->getSite(
$sourceUrl->getParsedUrl()['host'] ) !== null &&
- $this->getTitleFromSourceUrl( $sourceUrl ) !== null;
+ $site = $this->siteTableSiteLookup->getSite(
$sourceUrl->getParsedUrl()['host'] );
+
+ if ( $site === null ) {
+ $this->logger->error( __METHOD__ . ' failed site check
for URL: ' . $sourceUrl->getUrl() );
+ return false;
+ }
+
+ $titleString = $this->getTitleFromSourceUrl( $sourceUrl );
+
+ if ( $titleString === null ) {
+ $this->logger->error( __METHOD__ . ' failed title check
for URL: ' . $sourceUrl->getUrl() );
+ return false;
+ }
+
+ return true;
}
/**
diff --git a/src/ServiceWiring.php b/src/ServiceWiring.php
index 4de81ae..5acde84 100644
--- a/src/ServiceWiring.php
+++ b/src/ServiceWiring.php
@@ -64,9 +64,9 @@
$importer = new Importer(
$wikiRevisionFactory,
$httpRequestExecutor,
- $uploadBaseFactory
+ $uploadBaseFactory,
+ LoggerFactory::getInstance( 'FileImporter' )
);
- $importer->setLogger( LoggerFactory::getInstance(
'FileImporter' ) );
return $importer;
},
@@ -106,19 +106,19 @@
*/
$httpApiLookup = $services->getService(
'FileImporterMediaWikiHttpApiLookup' );
$httpRequestExecutor = $services->getService(
'FileImporterHttpRequestExecutor' );
-
- $detailRetriever = new Remote\MediaWiki\ApiDetailRetriever(
- $httpApiLookup, $httpRequestExecutor
- );
- $detailRetriever->setLogger( LoggerFactory::getInstance(
'FileImporter' ) );
-
- // TODO ApiImportTitleChecker here should have a logger....
+ $logger = LoggerFactory::getInstance( 'FileImporter' );
$site = new SourceSite(
new AnyMediaWikiFileUrlChecker(),
- $detailRetriever,
+ new Remote\MediaWiki\ApiDetailRetriever(
+ $httpApiLookup,
+ $httpRequestExecutor,
+ $logger
+ ),
new Remote\MediaWiki\RemoteApiImportTitleChecker(
- $httpApiLookup, $httpRequestExecutor
+ $httpApiLookup,
+ $httpRequestExecutor,
+ $logger
),
new SourceUrlNormalizer( function ( SourceUrl
$sourceUrl ) {
return new SourceUrl( str_replace( '..GOAT..',
'', $sourceUrl->getUrl() ) );
@@ -142,20 +142,22 @@
$siteTableLookup = $services->getService(
'FileImporterMediaWikiSiteTableSiteLookup' );
$httpApiLookup = $services->getService(
'FileImporterMediaWikiHttpApiLookup' );
$httpRequestExecutor = $services->getService(
'FileImporterHttpRequestExecutor' );
-
- $detailRetriever = new Remote\MediaWiki\ApiDetailRetriever(
- $httpApiLookup, $httpRequestExecutor
- );
- $detailRetriever->setLogger( LoggerFactory::getInstance(
'FileImporter' ) );
-
- // TODO SiteTableSourceUrlChecker here should have a logger....
- // TODO ApiImportTitleChecker here should have a logger....
+ $logger = LoggerFactory::getInstance( 'FileImporter' );
$site = new SourceSite(
- new SiteTableSourceUrlChecker( $siteTableLookup ),
- $detailRetriever,
+ new SiteTableSourceUrlChecker(
+ $siteTableLookup,
+ $logger
+ ),
+ new Remote\MediaWiki\ApiDetailRetriever(
+ $httpApiLookup,
+ $httpRequestExecutor,
+ $logger
+ ),
new Remote\MediaWiki\RemoteApiImportTitleChecker(
- $httpApiLookup, $httpRequestExecutor
+ $httpApiLookup,
+ $httpRequestExecutor,
+ $logger
),
new SourceUrlNormalizer( function ( SourceUrl
$sourceUrl ) {
return new SourceUrl( str_replace( '.m.', '.',
$sourceUrl->getUrl() ) );
diff --git a/src/Services/Importer.php b/src/Services/Importer.php
index acbc775..da5128e 100644
--- a/src/Services/Importer.php
+++ b/src/Services/Importer.php
@@ -10,9 +10,7 @@
use FileImporter\Operations\TextRevisionFromTextRevision;
use FileImporter\Services\Http\HttpRequestExecutor;
use FileImporter\Services\UploadBase\UploadBaseFactory;
-use Psr\Log\LoggerAwareInterface;
use Psr\Log\LoggerInterface;
-use Psr\Log\NullLogger;
use RuntimeException;
use Title;
use User;
@@ -21,7 +19,7 @@
/**
* Performs an import of a file to the local wiki based on an ImportPlan
object for a given User.
*/
-class Importer implements LoggerAwareInterface {
+class Importer {
/**
* @var WikiRevisionFactory
@@ -43,23 +41,15 @@
*/
private $logger;
- /**
- * @param WikiRevisionFactory $wikiRevisionFactory
- * @param HttpRequestExecutor $httpRequestExecutor
- * @param UploadBaseFactory $uploadBaseFactory
- */
public function __construct(
WikiRevisionFactory $wikiRevisionFactory,
HttpRequestExecutor $httpRequestExecutor,
- UploadBaseFactory $uploadBaseFactory
+ UploadBaseFactory $uploadBaseFactory,
+ LoggerInterface $logger
) {
$this->wikiRevisionFactory = $wikiRevisionFactory;
$this->httpRequestExecutor = $httpRequestExecutor;
$this->uploadBaseFactory = $uploadBaseFactory;
- $this->logger = new NullLogger();
- }
-
- public function setLogger( LoggerInterface $logger ) {
$this->logger = $logger;
}
@@ -80,6 +70,8 @@
// TODO the type of ImportOperation created should be decided
somewhere
+ $this->logger->info( __METHOD__ . ' started' );
+
/**
* Text revisions should be added first. See T147451.
* This ensures that the page entry is created and if something
fails it can thus be deleted.
@@ -88,7 +80,8 @@
$importOperations->add( new
TextRevisionFromTextRevision(
$plannedTitle,
$textRevision,
- $this->wikiRevisionFactory
+ $this->wikiRevisionFactory,
+ $this->logger
) );
}
@@ -98,19 +91,24 @@
$fileRevision,
$this->httpRequestExecutor,
$this->wikiRevisionFactory,
- $this->uploadBaseFactory
+ $this->uploadBaseFactory,
+ $this->logger
) );
}
+ $this->logger->info( __METHOD__ . ' ImportOperations built.' );
+
if ( !$importOperations->prepare() ) {
- $this->logger->error( 'Failed to prepare operations.' );
+ $this->logger->error( __METHOD__ . 'Failed to prepare
operations.' );
throw new RuntimeException( 'Failed to prepare
operations.' );
}
+ $this->logger->info( __METHOD__ . ' operations prepared.' );
if ( !$importOperations->commit() ) {
- $this->logger->error( 'Failed to commit operations.' );
+ $this->logger->error( __METHOD__ . 'Failed to commit
operations.' );
throw new RuntimeException( 'Failed to commit
operations.' );
}
+ $this->logger->info( __METHOD__ . ' operations committed.' );
// TODO the below should be an ImportOperation
$articleIdForUpdate = $this->getArticleIdForUpdate( $importPlan
);
@@ -165,7 +163,8 @@
);
if ( !$editResult->isOK() ) {
- throw new RuntimeException( 'Failed to create import
edit' );
+ $this->logger->error( __METHOD__ . ' Failed to create
import revision.' );
+ throw new RuntimeException( 'Failed to create import
revision' );
}
}
@@ -189,6 +188,7 @@
);
if ( !$editResult->isOK() ) {
+ $this->logger->error( __METHOD__ . ' Failed to create
user edit.' );
throw new RuntimeException( 'Failed to create user
edit' );
}
}
diff --git a/src/SpecialImportFile.php b/src/SpecialImportFile.php
index 193cd22..53827d3 100644
--- a/src/SpecialImportFile.php
+++ b/src/SpecialImportFile.php
@@ -22,9 +22,11 @@
use FileImporter\Services\SourceSiteLocator;
use Html;
use ILocalizedException;
+use MediaWiki\Logger\LoggerFactory;
use MediaWiki\MediaWikiServices;
use Message;
use PermissionsError;
+use Psr\Log\LoggerInterface;
use SpecialPage;
use UploadBase;
use User;
@@ -47,6 +49,11 @@
*/
private $importPlanFactory;
+ /**
+ * @var LoggerInterface
+ */
+ private $logger;
+
private $config;
public function __construct() {
@@ -63,6 +70,7 @@
$this->sourceSiteLocator = $services->getService(
'FileImporterSourceSiteLocator' );
$this->importer = $services->getService( 'FileImporterImporter'
);
$this->importPlanFactory = $services->getService(
'FileImporterImportPlanFactory' );
+ $this->logger = LoggerFactory::getInstance( 'FileImporter' );
}
public function getGroupName() {
@@ -121,9 +129,12 @@
return;
}
+ $clientUrl = $this->getRequest()->getVal( 'clientUrl' );
try {
+ $this->logger->info( 'Getting ImportPlan for URL: ' .
$clientUrl );
$importPlan = $this->getImportPlan();
} catch ( ImportException $exception ) {
+ $this->logger->info( 'ImportException: ' .
$exception->getMessage() );
$this->handleImportException( $exception );
return;
}
diff --git a/tests/phpunit/MediaWiki/SiteTableSourceUrlCheckerTest.php
b/tests/phpunit/MediaWiki/SiteTableSourceUrlCheckerTest.php
index 7a77924..3b0cdd3 100644
--- a/tests/phpunit/MediaWiki/SiteTableSourceUrlCheckerTest.php
+++ b/tests/phpunit/MediaWiki/SiteTableSourceUrlCheckerTest.php
@@ -7,6 +7,7 @@
use FileImporter\Remote\MediaWiki\SiteTableSourceUrlChecker;
use HashSiteStore;
use PHPUnit_Framework_TestCase;
+use Psr\Log\NullLogger;
use Site;
class SiteTableSourceUrlCheckerTest extends PHPUnit_Framework_TestCase {
@@ -21,7 +22,8 @@
}
return new SiteTableSourceUrlChecker(
- new SiteTableSiteLookup( new HashSiteStore( $sites ) )
+ new SiteTableSiteLookup( new HashSiteStore( $sites ) ),
+ new NullLogger()
);
}
diff --git a/tests/phpunit/Remote/MediaWiki/ApiDetailRetrieverTest.php
b/tests/phpunit/Remote/MediaWiki/ApiDetailRetrieverTest.php
index 4ffe2df..8156ad3 100644
--- a/tests/phpunit/Remote/MediaWiki/ApiDetailRetrieverTest.php
+++ b/tests/phpunit/Remote/MediaWiki/ApiDetailRetrieverTest.php
@@ -10,6 +10,7 @@
use MWHttpRequest;
use PHPUnit_Framework_TestCase;
use Exception;
+use Psr\Log\NullLogger;
class ApiDetailRetrieverTest extends PHPUnit_Framework_TestCase {
@@ -36,7 +37,8 @@
public function testInvalidResponse( $content, Exception $expected ) {
$service = new ApiDetailRetriever(
$this->getMockHttpApiLookup(),
- $this->getMockHttpRequestExecutor( 'File:Foo.jpg',
json_encode( $content ) )
+ $this->getMockHttpRequestExecutor( 'File:Foo.jpg',
json_encode( $content ) ),
+ new NullLogger()
);
$this->setExpectedException( get_class( $expected ),
$expected->getMessage() );
@@ -113,7 +115,8 @@
public function testValidResponse( $sourceUrl, $titleString, $content,
$expected ) {
$service = new ApiDetailRetriever(
$this->getMockHttpApiLookup(),
- $this->getMockHttpRequestExecutor( $titleString,
$content )
+ $this->getMockHttpRequestExecutor( $titleString,
$content ),
+ new NullLogger()
);
$importDetails = $service->getImportDetails( $sourceUrl );
--
To view, visit https://gerrit.wikimedia.org/r/392411
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I9c5f6d977c62b17c3ef3d92d0d4580e2c1e9fbb3
Gerrit-PatchSet: 7
Gerrit-Project: mediawiki/extensions/FileImporter
Gerrit-Branch: master
Gerrit-Owner: Addshore <[email protected]>
Gerrit-Reviewer: Addshore <[email protected]>
Gerrit-Reviewer: Andrew-WMDE <[email protected]>
Gerrit-Reviewer: Reedy <[email protected]>
Gerrit-Reviewer: Tobias Gritschacher <[email protected]>
Gerrit-Reviewer: WMDE-Fisch <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
