Updating iegreview codebase to use wikimedia slimapp library
ClosedPublic

Authored by Nehajha on Nov 26 2017, 6:31 PM.

Details

Maniphest Tasks
T114969: Update grantreview codebase to use the wikimedia/slimapp library
Reviewers
Niharika
bd808
Commits
rWIEG65967e170dfa: Updating iegreview codebase to use wikimedia slimapp library
Patch without arc
git checkout -b D894 && curl -L https://phabricator.wikimedia.org/D894?download=true | git apply
Summary

Added wikimedia slimapp library, use slimapp classes and remove shared code

Diff Detail

Repository
rWIEG wikimedia-iegreview
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
Nehajha created this revision.Nov 26 2017, 6:31 PM
Nehajha requested review of this revision.Nov 26 2017, 6:31 PM
In D894#17791, @jenkins wrote:

The test failure seems to be a problem with the git clone of the repo on the Jenkins host:

0:00:01.027 ERROR: Error cloning remote repo 'origin'
00:00:01.027 hudson.plugins.git.GitException: Command "git fetch --tags --progress https://phabricator.wikimedia.org/diffusion/WIEG/iegreview.git +refs/heads/*:refs/remotes/origin/*" returned status code 128:
00:00:01.027 stdout: 
00:00:01.027 stderr: fatal: unable to access 'https://phabricator.wikimedia.org/diffusion/WIEG/iegreview.git/': The requested URL returned error: 403

@hashar can you help us debug this failure? Its not clear to me what is misconfigured and causing the clone to fail.

wikimedia/iegreview.git is read-only in Gerrit and has been migrated to Phabricator. However it is marked as inactive on https://phabricator.wikimedia.org/source/iegreview/

Looking at the repository history:

@demon deactivated this repository.
Thu, Nov 9, 5:50 PM

I have activated it again and manually scheduled an update.

I went to the failling build at https://phabricator.wikimedia.org/B2547 and clicked the Restart All Builds. It seems to fail on PHP_CodeSniffer

00:00:34.382 PHP CODE SNIFFER REPORT SUMMARY
00:00:34.382 ----------------------------------------------------------------------
00:00:34.382 FILE                                                  ERRORS  WARNINGS
00:00:34.382 ----------------------------------------------------------------------
00:00:34.382 src/App.php                                              1       0
00:00:34.382 UserData.php                                             1       0
00:00:34.382 ----------------------------------------------------------------------
00:00:34.382 A TOTAL OF 2 ERRORS AND 0 WARNINGS WERE FOUND IN 2 FILES
00:00:34.382 ----------------------------------------------------------------------
00:00:34.382 PHPCBF CAN FIX 2 OF THESE SNIFF VIOLATIONS AUTOMATICALLY
00:00:34.382 ----------------------------------------------------------------------

Both being:

Multiple empty lines should not exist in a row; found 2 consecutive empty lines
(MediaWiki.WhiteSpace.MultipleEmptyLines.MultipleEmptyLines)

@hashar That's an error on my part. I will fix it. Thank you so much for your help :)

Nehajha updated this revision to Diff 2356.Nov 27 2017, 9:09 AM

remove consecutive emty lines

Fixed ! :-]

demon removed a subscriber: demon.Nov 27 2017, 7:27 PM
bd808 requested changes to this revision.Jan 1 2018, 6:11 AM

The changes to the vendor directory are missing from this patch. My local testing is also complaining of the composer.lock file being aout of date with respect to the composer.json file. I'm not entirely sure if this is caused by a difference in the local composer version used or not. The vendor files being directly embedded in this git repo cause some interesting challenges. To prepare your local git repo you need to do something like composer update && rm -rf vendor && composer install --no-dev && git add vendor. This will make a complete composer.lock that can be used for local testing and CI testing (includes pinned require-dev values) but then only add the libraries needed for production deployment to the git repo.

After I updated and then reinstalled using the composer commands given above locally things seem to work as expected, so I think this is all that needs to be changed before we can accept the patch.

This revision now requires changes to proceed.Jan 1 2018, 6:11 AM
Nehajha updated this revision to Diff 2456.Jan 1 2018, 7:40 AM

added vendor directory and complete composer.json

bd808 requested changes to this revision.Jan 1 2018, 8:01 PM

composer install (full dev dependency install from composer.lock) is now including PHP 7.0 requirements:

$ composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Your requirements could not be resolved to an installable set of packages.

  Problem 1
    - Installation request for phpdocumentor/reflection-docblock 4.2.0 -> satisfiable by phpdocumentor/reflection-docblock[4.2.0].
    - phpdocumentor/reflection-docblock 4.2.0 requires php ^7.0 -> your PHP version (5.6.30) does not satisfy that requirement.
  Problem 2
    - phpdocumentor/reflection-docblock 4.2.0 requires php ^7.0 -> your PHP version (5.6.30) does not satisfy that requirement.
    - phpspec/prophecy 1.7.3 requires phpdocumentor/reflection-docblock ^2.0|^3.0.2|^4.0 -> satisfiable by phpdocumentor/reflection-docblock[4.2.0].
    - Installation request for phpspec/prophecy 1.7.3 -> satisfiable by phpspec/prophecy[1.7.3].

This is caused by the composer update having been run with PHP 7.0+. The host that we deploy to in production (krypton.eqiad.wmnet) is running Debian Jessie and PHP 5.6.30. Technically having PHP 7.0 dependencies in the lock file for running tests is something we can live with, but its a little spooky. The easiest way to fix the lockfile would be to run composer update && composer install --no-dev from a Debian Jessie host (e.g. a Jessie MediaWiki-Vagrant VM).

This revision now requires changes to proceed.Jan 1 2018, 8:01 PM
Nehajha updated this revision to Diff 2459.Jan 2 2018, 10:30 AM

Fixed composer.json

@hashar Do you know if there's a way to see individual diffs yet? Clicking on it takes me to https://phabricator.wikimedia.org/D894?id=2459 (for example) but doesn't actually show me that specific diff.

Niharika added inline comments.Jan 3 2018, 10:09 AM
src/Auth/AuthManager.php
63

Spacing issue here. Needs two tab-spaces. Same in the else block below.

I try to apply the patch locally but I get this set of errors -

<stdin>:1541: space before tab in indent.
	   	return $user->isAdmin() || $user->canViewReports();
<stdin>:1543: space before tab in indent.
	   	return false;
<stdin>:4784: trailing whitespace.
        'W' => 
<stdin>:4790: trailing whitespace.
        'T' => 
<stdin>:4794: trailing whitespace.
        'S' => 
error: patch failed: public/index.php:40
error: public/index.php: patch does not apply

Due to these it doesn't even download the patch in my branch. Then I have to load all files here to find out where the error happened. The numbers don't seem to make much sense. @hashar would you know if there is a way to disable lint-checks when I am downloading patches for review? Or a different arc command for it?

Due to these it doesn't even download the patch in my branch. Then I have to load all files here to find out where the error happened. The numbers don't seem to make much sense. @hashar would you know if there is a way to disable lint-checks when I am downloading patches for review? Or a different arc command for it?

You can always use the "Patch without arc" described in the header: git checkout -b D894 && curl -L https://phabricator.wikimedia.org/D894?download=true | git apply

I try to apply the patch locally but I get this set of errors -

I can't recreate this or the various errors @Niharika comment on:

$ arc patch D894
Created and checked out branch arcpatch-D894.
Downloading binary data for 'phar-sample.phar'...
Checking patch vendor/wikimedia/slimapp/src/TwigExtension.php...
(... snip ...)
Applied patch composer.json cleanly.
 OKAY  Successfully committed patch.
$ composer install
Loading composer repositories with package information
Installing dependencies (including require-dev) from lock file
Package operations: 24 installs, 0 updates, 0 removals
  - Installing jakub-onderka/php-parallel-lint (v0.9.2): Loading from cache
(... snip ...)
  - Installing phpunit/phpunit (4.8.36): Loading from cache
jakub-onderka/php-parallel-lint suggests installing jakub-onderka/php-console-highlighter (Highlight syntax in code snippet)
symfony/yaml suggests installing symfony/console (For validating YAML files using the lint command)
sebastian/global-state suggests installing ext-uopz (*)
phpunit/phpunit suggests installing phpunit/php-invoker (~1.1)
Generating autoload files
$ composer test
> composer lint
> parallel-lint . --exclude vendor
PHP 5.6.30 | 10 parallel jobs
....................................                         36/36 (100 %)


Checked 36 files in 0.2 seconds
No syntax error found
> phpunit $PHPUNIT_ARGS
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.

Runtime:        PHP 5.6.30 with Xdebug 2.5.5
Configuration:  /Users/bd808/projects/wmf/vagrant-disposable/srv/iegreview/phpunit.xml.dist

TAP version 13
.ok 1 - Wikimedia\IEGReview\ArrayTest::testDifference with data set #0 (array(1, 2, 3), array(4, 5, 6), array(4, 5, 6), array(1, 2, 3))
.ok 2 - Wikimedia\IEGReview\ArrayTest::testDifference with data set #1 (array(1, 2, 3, 4), array(1, 2, 3), array(), array(4))
.ok 3 - Wikimedia\IEGReview\ArrayTest::testDifference with data set #2 (array(1, 2, 3), array(1, 2, 3, 4), array(4), array())
.ok 4 - Wikimedia\IEGReview\ArrayTest::testDifference with data set #3 (array(1, 2, 3), array(1, 2, 3), array(), array())
.ok 5 - Wikimedia\IEGReview\ArrayTest::testDifference with data set #4 (array(1, 2, 3, 4), array(2, 3, 4, 5), array(5), array(1))
.ok 6 - Wikimedia\IEGReview\ArrayTest::testDifference with data set #5 (array(), array(), array(), array())
.ok 7 - Wikimedia\IEGReview\ArrayTest::testDifference with data set #6 (array('Hello world'), array('Test string'), array('Test string'), array('Hello world'))
.ok 8 - Wikimedia\IEGReview\ArrayTest::testDifference with data set #7 (array('A', 'B'), array('C', 'A'), array('C'), array('B'))
.ok 9 - Wikimedia\IEGReview\ArrayTest::testDifferenceException with data set #0 ('Lorem Ipsum', 'Ipsum Test')
.ok 10 - Wikimedia\IEGReview\ArrayTest::testDifferenceException with data set #1 ('', '')
1..10


Time: 101 ms, Memory: 5.00MB

OK (10 tests, 20 assertions)
> composer phpcs
> phpcs $PHPCS_ARGS
....................................

Time: 3.3 secs; Memory: 16.5Mb

Looking at files like src/Auth/AuthManager.php in my editor I can now see mixed space/tab issues that @Niharika was commenting on. I'm not sure why phpcs isn't flagging them as errors however. Maybe because we are running such an old version of it?

I manually grepped and found these "line staring with tab followed by a space" errors:

$ git grep -n '^    [^*]'
src/Auth/AuthManager.php:40:    $user = $this->getUserData();
src/Auth/AuthManager.php:41:    return $user ? $user->isAdmin() : false;
src/Auth/AuthManager.php:50:    $user = $this->getUserData();
src/Auth/AuthManager.php:51:    return $user ? $user->isReviewer() : false;
src/Auth/AuthManager.php:60:    $user = $this->getUserData();
src/Auth/AuthManager.php:61:    if ( $user ) {
src/Auth/AuthManager.php:62:     return $user->isAdmin() || $user->canViewReports();
src/Auth/AuthManager.php:63:    } else {
src/Auth/AuthManager.php:64:     return false;
src/Auth/AuthManager.php:65:    }
src/Dao/Reports.php:76:    }, $questions
vendor/twig/twig/ext/twig/twig.c:1082:       $ret = call_user_func_array(array($object, $method), $arguments);
vendor/twig/twig/ext/twig/twig.c:1084:       if ($call && ($ignoreStrictCheck || !$this->env->isStrictVariables())) {
vendor/twig/twig/ext/twig/twig.c:1085:           return null;
vendor/twig/twig/ext/twig/twig.c:1086:       }
vendor/twig/twig/ext/twig/twig.c:1087:       throw $e;

To get the literal tab in the grep command, type control+V and then (tab). The ones showing up in vendor can be ignored.

In D894#18794, @bd808 wrote:

Due to these it doesn't even download the patch in my branch. Then I have to load all files here to find out where the error happened. The numbers don't seem to make much sense. @hashar would you know if there is a way to disable lint-checks when I am downloading patches for review? Or a different arc command for it?

You can always use the "Patch without arc" described in the header: git checkout -b D894 && curl -L https://phabricator.wikimedia.org/D894?download=true | git apply

That's exactly what I did which caused those problems. I ran that command and it flagged those errors and exited without applying the patch.

Nehajha updated this revision to Diff 2472.EditedJan 4 2018, 3:44 PM

I actually couldn't reproduce any of these errors. Then I upgraded phpcs to different versions. There were errors but none of them were related to whitespace/tab. I have converted the if-else block to a simpler version for now. If that was the only problem then @Niharika will be able to test this.

diff --git c/src/Auth/AuthManager.php i/src/Auth/AuthManager.php
index d238cfa..8ec6753 100644
--- c/src/Auth/AuthManager.php
+++ i/src/Auth/AuthManager.php
@@ -37,8 +37,8 @@ class AuthManager extends \Wikimedia\Slimapp\Auth\AuthManager {
 	* otherwise
 	*/
 	public function isAdmin() {
-	   $user = $this->getUserData();
-	   return $user ? $user->isAdmin() : false;
+		$user = $this->getUserData();
+		return $user ? $user->isAdmin() : false;
 	}
 
 	/**
@@ -47,8 +47,8 @@ class AuthManager extends \Wikimedia\Slimapp\Auth\AuthManager {
 	* otherwise
 	*/
 	public function isReviewer() {
-	   $user = $this->getUserData();
-	   return $user ? $user->isReviewer() : false;
+		$user = $this->getUserData();
+		return $user ? $user->isReviewer() : false;
 	}
 
 	/**
@@ -57,8 +57,8 @@ class AuthManager extends \Wikimedia\Slimapp\Auth\AuthManager {
 	* otherwise
 	*/
 	public function canViewReports() {
-	   $user = $this->getUserData();
-	   return $user ? $user->isAdmin() || $user->canViewReports() : false;
+		$user = $this->getUserData();
+		return $user ? $user->isAdmin() || $user->canViewReports() : false;
 	}
 
 } // end AuthManager
diff --git c/src/Dao/Reports.php i/src/Dao/Reports.php
index 3ab566e..5cc97c4 100644
--- c/src/Dao/Reports.php
+++ i/src/Dao/Reports.php
@@ -72,8 +72,10 @@ class Reports extends AbstractDao {
 		$params = array_merge( $defaults, $params );
 
 		$questionIds = array_map(
-			function ( $elm ) { return "q{$elm['id']}";
-	  }, $questions
+			function ( $elm ) {
+				return "q{$elm['id']}";
+			},
+			$questions
 		);
 		$validSorts = array_merge(
 			array( 'id', 'title', 'amount', 'theme', 'rcnt', 'pcnt' ),

If this was gerrit I would just amend with that diff. Diffusion does not support collaborative editing of patches as far as I recall, so I haven't tried to do that here. Other than these trivial formatting issues the patch seems to work and is ready to land.

bd808 requested changes to this revision.Jan 7 2018, 8:18 PM
This revision now requires changes to proceed.Jan 7 2018, 8:18 PM
Nehajha updated this revision to Diff 2480.Jan 8 2018, 8:36 AM

Fixed Formatting issues

bd808 accepted this revision.Mar 5 2018, 3:45 AM
This revision is now accepted and ready to land.Mar 5 2018, 3:45 AM
This revision was automatically updated to reflect the committed changes.