Author: matthew.britton
Description:
It would be nice if revisions could be marked as "sighted" through the API when the FlaggedRevisions extension is installed.
Version: unspecified
Severity: enhancement
• bzimport | |
Nov 8 2008, 1:58 AM |
F5243: bug16278.patch | |
Nov 21 2014, 10:23 PM |
Author: matthew.britton
Description:
It would be nice if revisions could be marked as "sighted" through the API when the FlaggedRevisions extension is installed.
Version: unspecified
Severity: enhancement
Status | Subtype | Assigned | Task | ||
---|---|---|---|---|---|
Resolved | Catrope | T18633 Write API issues (tracking) | |||
Resolved | Catrope | T18278 FlaggedRevisions extension: API "sighting" of revisions |
patch to add ApiReview.php (action=review)
The proposed solution adds an API module ApiReview.php, which essentially imitates the behavior of RevisionReview::AjaxReview. To make it work, it has to make two changes to other FlaggedRevs files:
*The code within FlaggedArticle::addQuickReview to generate the template and image parameters had to be factored out to its own function to avoid code duplication.
*RevisionReview::submit had to be changed to a public function to be accessible from the api.
Caveat: The revision being reviewed has to be parsed/fetched from parser cache on API call. Don't know if this will be a perfomance problem.
attachment bug16278.patch ignored as obsolete
(In reply to comment #2)
Created an attachment (id=5601) [details]
patch to add ApiReview.php (action=review)
Why the ==/=== change in ApiBase.php?
The proposed solution adds an API module ApiReview.php, which essentially
imitates the behavior of RevisionReview::AjaxReview. To make it work, it has to
make two changes to other FlaggedRevs files:
*The code within FlaggedArticle::addQuickReview to generate the template and
image parameters had to be factored out to its own function to avoid code
duplication.
*RevisionReview::submit had to be changed to a public function to be accessible
from the api.
I haven't reviewed the FlaggedRevs changes, Aaron should do that.
Remarks on ApiReview.php:
'flags' => array(
ApiBase :: PARAM_ISMULTI => true, ApiBase :: PARAM_TYPE => $allowed_flags
),
where $allowed_flags is an array of allowed flags. You can then get these by iterating over $params['flags'], or by using $flags = array_flip($params['flags']); and using if(isset($flags['foo']))
Caveat: The revision being reviewed has to be parsed/fetched from parser cache
on API call. Don't know if this will be a perfomance problem.
It looks kind of pointless to me: you're only parsing the page to see which images and templates are used, right? Why not get that info from the imagelinks and templatelinks tables? Does the existing FlaggedRevs code parse stuff too?
(In reply to comment #3)
Why the ==/=== change in ApiBase.php?
Forgot to mention it. Currently, if a parameter has array( 0, ...) as parameter type, the description says "Can be empty or..." instead of showing 0 as possible value.
- You're doing an awful lot of permissions checking and validation that's
probably duplicated in RevisionView::AjaxReview(). Instead of duplicating this
code, it should be moved to a backend function called by both AjaxReview and
ApiReview.
True. I first considered using AjaxReview directly, but that would probably have been even more ugly.
- Use a boolean parameter instead of a 0/1 parameter for approve
- You can do this with 'approve' => false
- Setting &approve=anything produces $params['approve'] === true, not setting
&approve produces $params['approve'] === false
Could do that, but would have to cast it to an integer later anyway, as RevisionReview expects 0 or 1 as value.
- The variable parameter thing kind of gives me the creeps here. It would be
nicer to replace all the flag_* parameters by one multivalue parameter 'flags',
with a variable list of allowed values:'flags' => array(
ApiBase :: PARAM_ISMULTI => true, ApiBase :: PARAM_TYPE => $allowed_flags),
where $allowed_flags is an array of allowed flags. You can then get these by
iterating over $params['flags'], or by using $flags =
array_flip($params['flags']); and using if(isset($flags['foo']))
Hmm, I don't quite get that. Let's say we have 3 flags named A, B and C. A can have the values 0 or 1, B can have 0-2 and C 0-3. How would that work with multivalue params? Do you mean like it is done in ApiProtect and the expiry parameters?
Caveat: The revision being reviewed has to be parsed/fetched from parser cache
on API call. Don't know if this will be a perfomance problem.It looks kind of pointless to me: you're only parsing the page to see which
images and templates are used, right? Why not get that info from the imagelinks
and templatelinks tables? Does the existing FlaggedRevs code parse stuff too?
The existing FlaggedRevs code can take the parameters from the parser output, as it displays a from on the already parsed page. I don't know if one could get all needed parameters from the *links tables, will have to ask Aaron about it.
(In reply to comment #4)
It looks kind of pointless to me: you're only parsing the page to see which
images and templates are used, right? Why not get that info from the imagelinks
and templatelinks tables? Does the existing FlaggedRevs code parse stuff too?The existing FlaggedRevs code can take the parameters from the parser output,
as it displays a form on the already parsed page. I don't know if one could get
all needed parameters from the *links tables, will have to ask Aaron about it.
On second thought, we can't take the data from the *links tables, because a) they aren't always consistent and b) we can also flag old revisions, for which the data of the *link tables is obviously incorrect.
changed parameter approve->unapprove, simplified code for extracting flag_parameters a bit
attachment bug16278.patch ignored as obsolete
(In reply to comment #6)
Created an attachment (id=5602) [details]
changed parameter approve->unapprove, simplified code for extracting
flag_parameters a bit
The API part looks sane, but of course a lot of stuff should still be pushed to the backend.
adapted patch to the changes of r44861
Thanks. I changed the patch accordingly.
attachment bug16278.patch ignored as obsolete
(In reply to comment #4)
(In reply to comment #3)
Why the ==/=== change in ApiBase.php?
Forgot to mention it. Currently, if a parameter has array( 0, ...) as parameter
type, the description says "Can be empty or..." instead of showing 0 as
possible value.
Fixed in r44864.
Another fix: Only try parser cache, when reviewing the current revision of a page
Attached: