EBernhardson has uploaded a new change for review.
https://gerrit.wikimedia.org/r/295021
Change subject: Add phan and cleanup some found warnings
......................................................................
Add phan and cleanup some found warnings
This still ends up complaining a bunch about OAuthProvider and
RelevanceScoringProvider. Unfortunately because of how the Pimple DI
container works it's not really possible to fix these without throwing a
bunch of supress annotations arround. Annoyingly there is also the
problem that phan @suppress annotations only work on the closest
function definition, which means each service would have to be marked
with the annotations which is just too verbose. For the moment they are
just throwing errors.
Change-Id: Ic16905576311717ff957f5e3c62c02e240778304
---
A scripts/postprocess-phan.php
A scripts/run-phan.sh
M src/OAuth/MediaWiki.php
M src/RelevanceScoring/Assert/MinimumSubmittedValidator.php
M src/RelevanceScoring/Console/CacheClear.php
M src/RelevanceScoring/Controller/QueriesController.php
M src/RelevanceScoring/Import/Importer.php
M src/RelevanceScoring/RelevanceScoringProvider.php
M src/RelevanceScoring/Repository/QueriesRepository.php
M src/RelevanceScoring/Repository/ResultsRepository.php
M src/RelevanceScoring/Repository/ScoresRepository.php
M src/RelevanceScoring/Repository/ScoringQueueRepository.php
12 files changed, 160 insertions(+), 16 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/wikimedia/discovery/discernatron
refs/changes/21/295021/1
diff --git a/scripts/postprocess-phan.php b/scripts/postprocess-phan.php
new file mode 100644
index 0000000..e11cd25
--- /dev/null
+++ b/scripts/postprocess-phan.php
@@ -0,0 +1,29 @@
+<?php
+
+$results = file( "php://stdin" );
+foreach ( $results as $error ) {
+ if ( !preg_match( '/^(.*):(\d+) (Phan\w+) (.*)$/', $error, $matches ) )
{
+ echo "Failed to parse line: $error\n";
+ continue;
+ }
+ list( $source, $file, $lineno, $type, $message ) = $matches;
+ $errors[$file][] = array(
+ 'orig' => $error,
+ // convert from 1 indexed to 0 indexed
+ 'lineno' => $lineno - 1,
+ 'type' => $type,
+ );
+}
+
+foreach ( $errors as $file => $errors ) {
+ $source = file( $file );
+ foreach ( $errors as $error ) {
+ if ( $error['lineno'] === 0 || !preg_match(
+ "|/\* @suppress {$error["type"]} |",
+ $source[$error['lineno'] - 1]
+ ) ) {
+ echo $error['orig'];
+ }
+ }
+}
+
diff --git a/scripts/run-phan.sh b/scripts/run-phan.sh
new file mode 100755
index 0000000..9220b15
--- /dev/null
+++ b/scripts/run-phan.sh
@@ -0,0 +1,86 @@
+#!/bin/bash
+
+# Some systems, like mediawiki-vagrant, don't have realpath
+if ! which realpath > /dev/null; then
+ realpath() {
+ php -r "echo realpath('$*');"
+ }
+fi
+
+PROJECT_ROOT=$(realpath "${PROJECT_ROOT:-$(dirname "$0")/..}")
+PROJECT="src/" # tests/
+DEPS="vendor/"
+
+set -e
+
+if [ ! -f "$PROJECT_ROOT/app.php" ]; then
+ echo "Could not find installation at $PROJECT_ROOT"
+ echo "Please specify with PROJECT_ROOT environment variable"
+ echo
+ exit 1
+fi
+
+if ! which docker > /dev/null; then
+ echo "Docker not installed. Press any key to install docker or Ctrl-C
to quit"
+ read -n 1
+ sudo apt-get -y install docker.io
+ if [ -d /vagrant ]; then
+ # May also be required elsewhere..but not comfortable just
installing
+ # cgroup-lite to random peoples machines.
+ sudo apt-get -y install cgroup-lite
+ fi
+fi
+
+if ! id -Gn | grep docker > /dev/null; then
+ sudo adduser $(id -un) docker
+ echo "User added to docker group. You need to log out and log back in
to continue."
+ echo
+ exit 1
+fi
+
+if ! docker images | grep cloudflare/phan > /dev/null; then
+ git clone https://github.com/cloudflare/docker-phan.git
/tmp/docker-phan.$$
+ pushd /tmp/docker-phan.$$
+ ./build
+ popd
+ # Once build we can safely remove this repo
+ rm -rf /tmp/docker-phan.$$
+fi
+
+for i in $PROJECT; do
+ ALL_DIRS="$ALL_DIRS $PROJECT_ROOT/$i"
+done
+for i in $DEPS; do
+ SKIP_ANALYSIS="$SKIP_ANALYSIS,/mnt/src/$i"
+ ALL_DIRS="$ALL_DIRS $PROJECT_ROOT/$i"
+done
+# Strip leading comma
+SKIP_ANALYSIS="${SKIP_ANALYSIS:1}"
+
+PHAN_IN=$PROJECT_ROOT/phan.in.$$
+SED_PATTERN=$(echo $PROJECT_ROOT | sed 's/[\/&]/\\&/g')
+find $ALL_DIRS -iname '*.php' | sed "s/${SED_PATTERN}/\/mnt\/src/" > $PHAN_IN
+
+docker run \
+ --volume="$PROJECT_ROOT:/mnt/src" \
+ --rm \
+ --user "$(id -u):$(id -g)" \
+ cloudflare/phan:latest \
+ --file-list "/mnt/src/phan.in.$$" \
+ --exclude-directory-list "$SKIP_ANALYSIS" \
+ --output "php://stdout" \
+ | sed "s/\/mnt\/src/$SED_PATTERN/" \
+ | php $(dirname $0)/postprocess-phan.php \
+ | sed "s/$SED_PATTERN//" \
+ > /tmp/phan.out
+
+
+RETVAL=0
+if [ "$(wc -l < /tmp/phan.out)" -ne 0 ]; then
+ RETVAL=1
+fi
+
+cat /tmp/phan.out
+rm "$PHAN_IN" /tmp/phan.out
+exit $RETVAL
+
diff --git a/src/OAuth/MediaWiki.php b/src/OAuth/MediaWiki.php
index b916b26..6bc50fb 100644
--- a/src/OAuth/MediaWiki.php
+++ b/src/OAuth/MediaWiki.php
@@ -73,6 +73,11 @@
return $data->username;
}
+ /**
+ * @param string $body
+ *
+ * @return TokenCredentials
+ */
protected function createTokenCredentials($body)
{
parse_str($body, $data);
@@ -136,6 +141,7 @@
$this->cachedUserDetailsResponse = $decoded;
}
+ /* @suppress PhanTypeMismatchReturn parent class annotates this as
unknown, but we know better */
return $this->cachedUserDetailsResponse;
}
}
diff --git a/src/RelevanceScoring/Assert/MinimumSubmittedValidator.php
b/src/RelevanceScoring/Assert/MinimumSubmittedValidator.php
index 1a22640..6872f39 100644
--- a/src/RelevanceScoring/Assert/MinimumSubmittedValidator.php
+++ b/src/RelevanceScoring/Assert/MinimumSubmittedValidator.php
@@ -4,6 +4,7 @@
use Symfony\Component\Validator\Constraint;
use Symfony\Component\Validator\ConstraintValidator;
+use Symfony\Component\Validator\Exception\UnexpectedTypeException;
class MinimumSubmittedValidator extends ConstraintValidator
{
@@ -14,6 +15,7 @@
}
$have = count(array_filter($values, function ($val) { return $val !==
null; }));
+ /* @suppress PhanUndeclaredMethod Phan doesn't know this is a
MinimumSubmitted instance */
$allowed = $constraint->getMinimumAllowed(count($values));
if ($have < $allowed) {
diff --git a/src/RelevanceScoring/Console/CacheClear.php
b/src/RelevanceScoring/Console/CacheClear.php
index 6b641ab..20ecf3d 100644
--- a/src/RelevanceScoring/Console/CacheClear.php
+++ b/src/RelevanceScoring/Console/CacheClear.php
@@ -3,19 +3,23 @@
namespace WikiMedia\RelevanceScoring\Console;
use Knp\Command\Command;
+use RecursiveDirectoryIterator;
+use RecursiveIteratorIterator;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
-use Twig_Environment;
class CacheClear extends Command
{
- /** @var Twig_Environment */
- private $twig;
+ /** @var string */
+ private $cacheDir;
- public function __construct(Twig_Environment $twig)
+ /**
+ * @param string $cacheDir
+ */
+ public function __construct($cacheDir)
{
parent::__construct('cache:clear');
- $this->twig = $twig;
+ $this->cacheDir = $cacheDir;
}
protected function configure()
@@ -25,8 +29,16 @@
protected function execute(InputInterface $input, OutputInterface $output)
{
- $this->twig->clearCacheFiles();
- $output->writeln('Cache cleared');
+ $it = new RecursiveDirectoryIterator(
+ $this->cacheDir,
+ RecursiveIteratorIterator::LEAVES_ONLY
+ );
+ $it = new RecursiveIteratorIterator($it);
+ foreach ($it as $file) {
+ if ($file->isFile()) {
+ @unlink($file->getPathname());
+ }
+ }
return 0;
}
diff --git a/src/RelevanceScoring/Controller/QueriesController.php
b/src/RelevanceScoring/Controller/QueriesController.php
index fc37cc0..80b6f96 100644
--- a/src/RelevanceScoring/Controller/QueriesController.php
+++ b/src/RelevanceScoring/Controller/QueriesController.php
@@ -29,6 +29,8 @@
private $resultsRepo;
/** @var ScoresRepository */
private $scoresRepo;
+ /** @var ScoringQueueRepository */
+ private $scoringQueueRepo;
/** @var string[] */
private $wikis;
diff --git a/src/RelevanceScoring/Import/Importer.php
b/src/RelevanceScoring/Import/Importer.php
index 3b1ba79..331d475 100644
--- a/src/RelevanceScoring/Import/Importer.php
+++ b/src/RelevanceScoring/Import/Importer.php
@@ -38,7 +38,7 @@
$this->scoringQueueRepo = $scoringQueueRepo;
$this->wikis = $wikis;
$this->getters = $getters;
- $this->output = function () {};
+ $this->output = function ($line) {};
}
public function setOutput($callable)
diff --git a/src/RelevanceScoring/RelevanceScoringProvider.php
b/src/RelevanceScoring/RelevanceScoringProvider.php
index 139fa73..003ad4a 100644
--- a/src/RelevanceScoring/RelevanceScoringProvider.php
+++ b/src/RelevanceScoring/RelevanceScoringProvider.php
@@ -191,7 +191,9 @@
private function registerConsole(Application $app)
{
$app['search.console.cache-clear'] = function () use ($app) {
- return new CacheClear($app['twig']);
+ $options = $app['twig.options'];
+
+ return new CacheClear($options['cache']);
};
$app['search.console.import'] = function () use ($app) {
diff --git a/src/RelevanceScoring/Repository/QueriesRepository.php
b/src/RelevanceScoring/Repository/QueriesRepository.php
index 6cd00ba..462e812 100644
--- a/src/RelevanceScoring/Repository/QueriesRepository.php
+++ b/src/RelevanceScoring/Repository/QueriesRepository.php
@@ -44,10 +44,11 @@
return new None();
}
- $rand = mt_rand(0, $maxId);
+ $rand = mt_rand(0, (int) $maxId);
$qb = $this->db->createQueryBuilder()
->select('q.id')
->from('queries', 'q')
+ /* @suppress PhanTypeMismatchArgument DBAL is mis-annotated to
only take strings here */
->add('join', [
'q' => [
[
@@ -194,14 +195,14 @@
* @param string $wiki
* @param string $wiki
* @param string $query
- * @param bool $imported
+ * @param string $imported Set to 'imported' to mark as already imported
*
* @return int
*
* @throws DuplicateQueryException
* @throws RuntimeException
*/
- public function createQuery(User $user, $wiki, $query, $imported = false)
+ public function createQuery(User $user, $wiki, $query, $imported = '')
{
try {
$inserted = $this->db->insert('queries', [
@@ -219,7 +220,7 @@
throw new RuntimeException('Failed insert new query');
}
- return $this->db->lastInsertId();
+ return (int) $this->db->lastInsertId();
}
/**
@@ -267,10 +268,12 @@
if ($userId !== null) {
$sql .= ' AND user_id = ?';
$params[] = $userId;
+ /* @suppress PhanUndeclaredClassConstant phan is missing this
constant */
$types[] = PDO::PARAM_INT;
}
$sql .= ' LIMIT ?';
$params[] = $limit;
+ /* @suppress PhanUndeclaredClassConstant phan is missing this constant
*/
$types[] = PDO::PARAM_INT;
return $this->db->fetchAll($sql, $params, $types);
diff --git a/src/RelevanceScoring/Repository/ResultsRepository.php
b/src/RelevanceScoring/Repository/ResultsRepository.php
index 6d0f6d5..e4a8a27 100644
--- a/src/RelevanceScoring/Repository/ResultsRepository.php
+++ b/src/RelevanceScoring/Repository/ResultsRepository.php
@@ -46,7 +46,7 @@
return new None();
}
- $rand = mt_rand(0, $maxId);
+ $rand = mt_rand(0, (int) $maxId);
$sql = <<<EOD
SELECT r.id
FROM results r
diff --git a/src/RelevanceScoring/Repository/ScoresRepository.php
b/src/RelevanceScoring/Repository/ScoresRepository.php
index 7440f2a..3494239 100644
--- a/src/RelevanceScoring/Repository/ScoresRepository.php
+++ b/src/RelevanceScoring/Repository/ScoresRepository.php
@@ -40,7 +40,7 @@
throw new RuntimeException('Failed inserting row');
}
- return $this->db->lastInsertId();
+ return (int) $this->db->lastInsertId();
}
public function getNumberOfScores(array $queryIds)
diff --git a/src/RelevanceScoring/Repository/ScoringQueueRepository.php
b/src/RelevanceScoring/Repository/ScoringQueueRepository.php
index 723edf0..7ab4553 100644
--- a/src/RelevanceScoring/Repository/ScoringQueueRepository.php
+++ b/src/RelevanceScoring/Repository/ScoringQueueRepository.php
@@ -136,6 +136,7 @@
}
});
+ /* @suppress PhanUndeclaredVariable Phan doesn't recognize this as
being defined above */
return $res;
}
@@ -151,8 +152,9 @@
$rowsDeleted = $this->db->delete('scoring_queue', $conditions);
if ($rowsDeleted > 1) {
- $this->logger->warn(
+ $this->logger->warning(
'Unexpectedly deleted {numRows} for query {query_id} and user
{user_id}',
+ /* @suppress PhanTypeMismatchArgument Phan doesn't understand
array addition */
$conditions + ['numRows' => $rowsDeleted]
);
} elseif ($rowsDeleted === 0) {
--
To view, visit https://gerrit.wikimedia.org/r/295021
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic16905576311717ff957f5e3c62c02e240778304
Gerrit-PatchSet: 1
Gerrit-Project: wikimedia/discovery/discernatron
Gerrit-Branch: master
Gerrit-Owner: EBernhardson <ebernhard...@wikimedia.org>
_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
