Add a method to PagePurger for purging multiple pages at once #36

karatakis · 2016-10-31T22:13:59Z

The function iterates through each page and calls the purge method

Bug T145364

The function iterates through each page and calls the purge method Bug: T145364

samwilson

Looks really great! I've just a couple of pedantic points to raise about documentation. :-)

The main thing missing I reckon is a mention of the multi-page purge feature (or the single one for that matter!) in docs/page_purger.rst. Could you add that?

samwilson · 2016-11-01T04:36:16Z

src/Service/PagePurger.php

@@ -43,6 +44,24 @@ public function purge( Page $page ) {
 	}

 	/**
+	 * @since 0.x


It'd be good to add a brief description to this method and to the purge() method, and perhaps also a longer description to both mentioning the existence of the other; makes it easier for people using autocomplete. (Brief descriptions are one-liners, and detailed descriptions are separated below these by a blank line; see http://www.stack.nl/~dimitri/doxygen/manual/docblocks.html for more.)

Also, I'd leave out the @since, because it's not clear yet what it'll be released as (although, @addshore do you have any guidelines about this? Are you using Semantic Versioning? If so, this should be @since 0.7.0 as that's the next minor version number that we'll hit).

Adding @SInCE 0.7 should be fine for now (Before releases I review everything anyway).
Semver is indeed used, but I have only been doing 0.x as in theory nothing would be added in a 0.x.>1 release.
Either 0.7 or 0.7.0 are both fine for me!

I will write a more detailed documentation for both functions

samwilson · 2016-11-01T04:40:24Z

src/Service/PagePurger.php

+	 * @param Pages $pages
+	 *
+	 * @return bool
+	 *


There shouldn't be an empty line after the @return tag. Also, it'd be good to explain the return value (however, see my comment below).

I will remove the empty line

samwilson · 2016-11-01T04:45:14Z

src/Service/PagePurger.php

+			$this->purge( $page );
+		}
+
+		return true;


What's the point of always returning true? I'd say either leave this out (and update the docblock to @return void) or have explain when this will return true and when other things might happen.

I realise that this is just following the pattern set by the purge() method, but I'd question that one too; perhaps we can make them both better?

I can return true/false if the request have succeed $this->api->postRequest(..) returns an array and I can check for the response. It is a good idea to apply this logic at purge() too.

Another idea is to return the response array, this idea gives more freedom but those who will use the library will have to do more check (if needed).

I think I would be against passing the raw result out of the method. But I don't feel too strongly really.
I think I prefer the true / false based on request success, although I guess to fit with everything else a request that fails would just throw an exception.
maybe void is the way to go?

Yeah, I think checking the result of the request would be best (and, as you say, that won't matter if the request throws an exception). I guess it just means that if no pages were purged, false will be returned? Actually, there are three results possible for each page: purged, missing, and invalid. Perhaps the purge methods should return true only when all results are purged and false if any are the other two?

addshore · 2016-11-01T09:54:53Z

src/Service/PagePurger.php

+	 * @return bool
+	 *
+	 */
+	public function purgePages( Pages $pages ) {


So, the more efficient way of implementing this method would be to do this in a single call.
See https://en.wikipedia.org/w/api.php?action=help&modules=purge
Multiple pages can be purged in a single request.

However I would be fine to merge this as is as an initial implementation.

Noted, I will implement it and update the pull request

Add comments and brief on both purge and purgePages. purgePages function now purge all pages in a single request by sumbiting at the 'purge' action all the pages ids in a multi-value seperation

addshore · 2016-11-02T08:33:26Z

src/Service/PagePurger.php

 	 *
-	 * @return bool
+	 * Purges all the pages of the Pages object
+	 * by submitting a 'purge' action to the wikipedia


mediawiki api

you are right 👍

addshore · 2016-11-02T08:36:50Z

src/Service/PagePurger.php

 		}

+		// convert an array to multiple-value format
+		// because the wikipedia api require multiple


mediawiki api

karatakis · 2016-11-02T21:23:27Z

src/Service/PagePurger.php

+	 *
+	 * @param Pages $pages the pages that are going to be purged
+	 *
+	 * @return bool


Is it a good idea to return boolean true if SimpleRequest have been successful or return the SimpleRequest response object/array ?

One possibility would be to return a Pages object containing only the successfully purged pages (i.e. the ones with a purged response). That way, people using this could check if the returned set of pages is the same as what they put in (and in some cases perhaps take other action for invalid or missing pages). This would make it hard to tell the difference between invalid and missing, though, which isn't so good. @addshore what do you think?

Does the API response list the pages that were successfully purged?
If so then yes perhaps returning a Pages object here would make sense!

Yep, it does (e.g.).

Yep, I think returning a Pages object sounds like a nice idea then!

Should I do the same with the purge method ?
or is it better to implement the logic to return true/false ?

I think for a single purge the boolean return may still make more sense?
But I don't have a strong opinion either way, returning a Page / null may also make sense!

I implemented the logic to return boolean, at the purge(...) function, it is easy to change to return Page object

* purge() implemented the logic to return true or false when the page was successfully purged * purgePages() return Pages object containing only the successfully purged pages

karatakis

Testing error

karatakis · 2016-11-03T21:03:06Z

src/Service/PagePurger.php

 			new SimpleRequest( 'purge', [ 'pageids' => $page->getId() ] )
 		);

-		return true;
+		// the purge response for the page
+		$purgeResponse = $responseArray['purge'][0];


This and the other occurance at line 95 make tests to fail, is it a good idea to place the following workaround ?

// if the purge action failed unexpected if ( !array_key_exists( 'purge' , $responseArray) ) { return false; / return new Pages(); }

Sounds good!

I will change the test to return real response

* updated testPurgePage. Now the postRequest method return a "real" response * added positive test path for purgePages

samwilson

Looks great! Just a couple of comments re testPurgePages.

samwilson · 2016-11-06T23:26:28Z

tests/unit/Service/PagePurgerTest.php

+			), new Page(
+				new PageIdentifier(
+					new Title( 'Bar', 1 ),
+					123


The two test pages shouldn't have the same ID. And is it worth adding in a third page, that won't be returned as purged (or do some other sort of extra test) just to make sure that missing and invalid responses are handled as we expect?

Noted, I will change the ID. Also I will create a new test for the case that a page is not purged

samwilson · 2016-11-06T23:28:25Z

tests/unit/Service/PagePurgerTest.php

+					new Title( 'Bar', 1 ),
+					123
+				),
+				new Revisions( [] )


If no $revisions parameter is passed, a new Revisions will be created in the Page constructor — so don't worry about passing this in.

Thanks for the information

we test if a non existent page is not returned in the purged Pages object after we run the purgePages command

karatakis · 2016-11-10T18:59:48Z

The only thing is missing now is the following document docs/page_purger.rst

addshore · 2016-11-11T18:12:16Z

@BlackSpirit96 do you feel like writing that one? :)

karatakis · 2016-11-12T12:50:22Z

I will write it :)

addshore · 2016-11-17T23:01:45Z

Bah, I should have / could have squashed that before merging, but oh well! it is in! :)

karatakis added 2 commits November 1, 2016 00:05

Add a method to PagePurger for purging multiple pages at once

d8ec697

The function iterates through each page and calls the purge method Bug: T145364

Fix identation

ea4e643

samwilson requested changes Nov 1, 2016

View reviewed changes

addshore reviewed Nov 1, 2016

View reviewed changes

Add comments and improve purgePages

59a6422

Add comments and brief on both purge and purgePages. purgePages function now purge all pages in a single request by sumbiting at the 'purge' action all the pages ids in a multi-value seperation

addshore requested changes Nov 2, 2016

View reviewed changes

Changed wikipedia to mediawiki

406e443

karatakis commented Nov 2, 2016

View reviewed changes

implement purgePages() return Pages object, purge() return boolean

46a834a

* purge() implemented the logic to return true or false when the page was successfully purged * purgePages() return Pages object containing only the successfully purged pages

karatakis commented Nov 3, 2016

View reviewed changes

Updated PagePurgerTest

12ea978

* updated testPurgePage. Now the postRequest method return a "real" response * added positive test path for purgePages

samwilson reviewed Nov 6, 2016

View reviewed changes

addshore approved these changes Nov 7, 2016

View reviewed changes

karatakis added 3 commits November 10, 2016 20:27

Fix phpcs errors

5aaa198

purgePages incorrect test

f4bd841

we test if a non existent page is not returned in the purged Pages object after we run the purgePages command

add incorrect test for purgePage

a02c697

added page_purger documentation

b17ffb5

addshore merged commit 59c99c3 into addwiki:master Nov 17, 2016

Add a method to PagePurger for purging multiple pages at once #36

Add a method to PagePurger for purging multiple pages at once #36

Conversation

karatakis commented Oct 31, 2016

samwilson 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karatakis Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

karatakis 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

samwilson 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

karatakis commented Nov 10, 2016

addshore commented Nov 11, 2016

karatakis commented Nov 12, 2016

addshore commented Nov 17, 2016

karatakis Nov 3, 2016 •

edited

Loading