Generalised caching, more documentation, and other set-up things #29

samwilson · 2016-10-13T18:36:46Z

This change adds a PSR-6 compatible caching system, instead of
only using Redis. This makes it easier to run locally (without
installing Redis).

Also extends the local-development setup instructions, making it
clearer what needs to be done to get things
up and running. Composer will now create a default .env file.

The Less cache is moved to the top-level 'cache' directory, because
this is where the cache is when Redis isn't used and because /src
seems like an odd place for volatile files. The new cache directory is added to the repo (empty) and a note about making it writable added to the readme.

The /login route is removed because it wasn't used anyway.

composer.lock is added, because then composer install will install the same thing for everyone, and the same in production (where composer update should never be run).

MusikAnimal

Big improvements! Left a few comments then also, in the super cool composer script, we should create the cache directory. Alternatively you could maybe add a .gitkeep file in /cache, then in .gitignore ignore everything in /cache except that file.

Another big thing that should have been in the README, we have to make the cache directory server-writable, which I think might involve a chmod 777

MusikAnimal · 2016-10-13T19:18:53Z

README.md

+   ```
+   $ mysqldump -u LABSYOU -p -P 4711 -h 127.0.0.1 --skip-lock-tables s51306__copyright_p > copyright.sql
+   $ mysql -u LOCALYOU -p -e "CREATE DATABASE copypatrol_dev;"
+   $ mysql -u LOCALYOU -p copypatrol_dev < copyright.sql


I would recommend connecting to the live copyright database through the same SSH tunnel, so in your .env you could use DB_DSN_PLAGIABOT = 'mysql:host=127.0.0.1;dbname=s51306__copyright_p;port=4711'

This way you don't need to recreate the data locally

I thought about that, but doesn't it need to update it also? Perhaps I've read things wrongly... if so, then yeah definitely no need to recreate locally! :-) Much easier.

The s51306__copyright_p table is what's being continually updated by EranBot, so you'll have a continuous flow of new data, at least until people stop adding copyright violations :) It is true our users go through the backlog pretty quickly, so if you wanted to test something against a lot of unreviewed cases you probably won't be able to. For most dev work I don't think this will be a problem, though

Cool, sounds good. I'm changing .env_example. Can a similar thing be done for the wikiproject database?

Goes to show how bad we've done with documentation over the course of development... the wikiproject database is no longer being used! So we can remove it entirely

ha! no worries. :-)

MusikAnimal · 2016-10-13T19:21:25Z

composer.json

+    ],
+    "post-update-cmd": [
+      "php -r \"file_exists('.env') || copy('.env_example', '.env');\""
+    ]


super cool!

Yes, just a shame that there's no post-update-or-install-cmd! :-)

MusikAnimal · 2016-10-13T19:27:46Z

src/App.php

+			}
+			$cache->setDriver( $driver );
+			return $cache;
+		} );


This is awesome! No disadvantage to having a local cache, but you could also make it work off the remote Redis via SSH tunnel ssh -L 4711:enwiki.labsdb:3306 -L 4712:tools-redis:6379 YOU@tools-dev.wmflabs.org then adjust .env accordingly. I should have added that to the README some time ago

This change adds a PSR-6 compatible caching system, instead of only using Redis. This makes it easier to run locally (without installing Redis). Also extends the local-development setup instructions, perhaps too verbosely, making it clearer what needs to be done to get things up and running. Composer will now create a default .env file. The Less cache is moved to the top-level 'cache' directory, becuase this is where the cache is when Redis isn't used and because /src seems like an odd place for volatile files. The /login route is removed because it wasn't used anyway.

Remove the notes about having a local database, add the cache directory (but not its contents), and add the composer.lock file so that everyone gets the same dependencies.

MusikAnimal · 2016-10-14T01:06:54Z

I'm not convinced the tests are working on Travis, unless it really takes 4+ hours... they pass locally so you've got my 👍. Might need another set of eyes? Not sure

samwilson · 2016-10-14T01:10:56Z

Yeah it's very strange. They took half an hour or so earlier, but reported themselves as 5 minutes or something.

I think the others should look at this first though. Just in case.

samwilson · 2016-10-14T15:10:33Z

Travis must've just been busy doing other things; ey got it to eventually.

samwilson · 2016-10-19T00:29:45Z

@Niharika29 would you mind casting your eye over this PR? Thanks!

MusikAnimal · 2016-10-21T03:56:20Z

.env_example

+
+; Caching system. If a Redis host is not defined, a local Filesystem cache will be used instead.
+REDIS_HOST = "tools-redis"
+REDIS_PORT = 6379


We might want to add SLIM_MODE = 'development' here, or at least I have on my local. Looking at the core Slim code I'm not convinced this does anything in particular by itself, but we might (or should) use it in other parts of the app, e.g. https://github.com/wikimedia/CopyPatrol/blob/master/src/Controllers/CopyPatrol.php#L364

Good idea. At the moment we're determining staging with $slim->view->set( 'staging', strpos( $rootUri, 'plagiabot' ) ); which could maybe be changed to a mode member of the container that uses SLIM_MODE (or APP_ENV? That seems a common name in some PHHP circles...).

And there's LOG_LEVEL and LOG_FILE too, which are good to set for development, to maybe include in this file.

MusikAnimal · 2016-10-21T03:58:34Z

.env_example

+OAUTH_SECRET_TOKEN   =
+OAUTH_ENDPOINT       = 'https://meta.wikimedia.org/w/index.php?title=Special:OAuth'
+OAUTH_REDIR          = 'https://meta.wikimedia.org/wiki/Special:OAuth/authenticate?'
+OAUTH_CALLBACK       = 'oob'


what is oob?

'Out Of Band', which isn't really correct because we do actually use a callback. (OOB usually means e.g. a desktop application is requesting the auth.) In our case, the callback is fixed by the consumer registration on Meta, so actually this will be ignored; it does have to be something though.

Interesting. Yeah for us it looks like we need a full URL to /copypatrol/oauth/callback. On my local I have the app running at localhost/copypatrol so I use http://localhost/copypatrol/oauth/callback and this works. We might suggest something similar here

MusikAnimal · 2016-10-21T04:07:33Z

FYI I've got this branch up and running on our staging environment and all seems well :) https://tools.wmflabs.org/plagiabot

kaldari

Everything looks awesome to me. Just had one question (see inline).

kaldari · 2016-10-22T01:52:44Z

composer.json

@@ -1,11 +1,13 @@
 {
  "license": "GPL-3.0+",
  "require": {
+    "ext-intl": "*",


What do we need the intl extension for?

That's for the number formatting I think. I did not know we could require PHP extensions through composer, so I ended up manually installing/enabling it my php.ini

Yes, it's for NumberFormatter. Composer can't install extensions but it does alert people to the fact that they are required.

Instead of requiring intl (which might require someone to spend half an hour upgrading their PHP), let's just check for the existence of NumberFormatter. After all, number formatting isn't critical to the tool's functionality:

if ( class_exists( 'NumberFormatter' ) ) { $formatter = new \NumberFormatter( $locale, \NumberFormatter::DECIMAL ); // Get separator symbols (include decimal as it is required and we might use it at some point) $decimal = $formatter->getSymbol( \NumberFormatter::DECIMAL_SEPARATOR_SYMBOL ); $thousands = $formatter->getSymbol( \NumberFormatter::GROUPING_SEPARATOR_SYMBOL ); } else { $decimal = '.'; $thousands = ','; }

Good idea! And we can make it simpler even, because the Twig defaults are the same as you've set there (dot and comma).

samwilson · 2016-10-22T05:15:05Z

I closed and opened this earlier because the Github Android app is rubbish. :-)

kaldari · 2016-10-22T07:42:37Z

src/App.php

@@ -179,6 +199,10 @@ protected function configureRoutes( \Slim\Slim $slim ) {
 				}
 			},
 			'twig-number-format' => function () use ( $slim ) {
+				if ( class_exists( \NumberFormatter::class ) ) {


!class_exists

Erk! Yes, well, hmm.... what can I say!?

:-)

(fixed now)

samwilson · 2016-10-25T18:16:36Z

Yes, it's for NumberFormatter. Composer can't inatall extensions but itdoes alert prople to their requirement.

Sent from my field telephone with K-9 Mail. Please excuse brevity.

On 22 October 2016 11:22:53 am AWST, MusikAnimal notifications@github.com wrote:

MusikAnimal commented on this pull request.

@@ -1,11 +1,13 @@
{
"license": "GPL-3.0+",
"require": {

"ext-intl": "*",

That's for the number formatting I think. I did not know we could
require PHP extensions through composer, so I ended up manually
installing/enabling it my php.ini

You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub:
#29

MusikAnimal requested changes Oct 13, 2016

View reviewed changes

samwilson added 4 commits October 13, 2016 14:32

Coding standards compliance

2b02b92

Redis isn't a hard requirement now

0573a9a

No local DB, composer.lock added, and cache info updated

42865ee

Remove the notes about having a local database, add the cache directory (but not its contents), and add the composer.lock file so that everyone gets the same dependencies.

MusikAnimal approved these changes Oct 14, 2016

View reviewed changes

Number fix.

14047dc

samwilson mentioned this pull request Oct 21, 2016

Don't assume everything is English Wikipedia (T145436) #32

Closed

Fix errors in example Oauth environment variables

e0f1d74

MusikAnimal reviewed Oct 21, 2016

View reviewed changes

samwilson added 2 commits October 21, 2016 13:13

Merge remote-tracking branch 'origin/master' into local-dev

8be5bf6

Merge remote-tracking branch 'origin/master' into local-dev

0b33d96

kaldari reviewed Oct 22, 2016

View reviewed changes

samwilson closed this Oct 22, 2016

samwilson reopened this Oct 22, 2016

Don't require intl extension, but check for it before use

8acac8b

kaldari requested changes Oct 22, 2016

View reviewed changes

Correct test for existance

5e28336

kaldari approved these changes Oct 24, 2016

View reviewed changes

MusikAnimal merged commit 6bd1969 into wikimedia:master Oct 24, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalised caching, more documentation, and other set-up things #29

Generalised caching, more documentation, and other set-up things #29

samwilson commented Oct 13, 2016 •

edited

MusikAnimal left a comment

MusikAnimal Oct 13, 2016

samwilson Oct 13, 2016

MusikAnimal Oct 13, 2016

samwilson Oct 13, 2016

MusikAnimal Oct 13, 2016

samwilson Oct 13, 2016

MusikAnimal Oct 13, 2016

samwilson Oct 13, 2016

MusikAnimal Oct 13, 2016

MusikAnimal commented Oct 14, 2016

samwilson commented Oct 14, 2016

samwilson commented Oct 14, 2016

samwilson commented Oct 19, 2016

MusikAnimal Oct 21, 2016 •

edited

samwilson Oct 21, 2016

MusikAnimal Oct 21, 2016

samwilson Oct 21, 2016

MusikAnimal Oct 21, 2016

MusikAnimal commented Oct 21, 2016

kaldari left a comment

kaldari Oct 22, 2016

MusikAnimal Oct 22, 2016

samwilson Oct 22, 2016

kaldari Oct 22, 2016

samwilson Oct 22, 2016

samwilson commented Oct 22, 2016

kaldari Oct 22, 2016

samwilson Oct 22, 2016

samwilson commented Oct 25, 2016

Generalised caching, more documentation, and other set-up things #29

Generalised caching, more documentation, and other set-up things #29

Conversation

samwilson commented Oct 13, 2016 • edited

MusikAnimal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MusikAnimal commented Oct 14, 2016

samwilson commented Oct 14, 2016

samwilson commented Oct 14, 2016

samwilson commented Oct 19, 2016

MusikAnimal Oct 21, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MusikAnimal commented Oct 21, 2016

kaldari left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samwilson commented Oct 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samwilson commented Oct 25, 2016

samwilson commented Oct 13, 2016 •

edited

MusikAnimal Oct 21, 2016 •

edited