-
Notifications
You must be signed in to change notification settings - Fork 6
v2.6.7 : Semantic Ask in tokens quick fix. Will be rewritten as a ful… #105
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: REL1_43
Are you sure you want to change the base?
Conversation
…l API call in 2.7.x
| // TODO: Rewrite this to be an proper API call for version 2.7.0 | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't forget this, as the current implementation is rather hacky.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep planned for next week
| $ret, | ||
| JSON_PRETTY_PRINT | ||
| ); | ||
| $database = MediaWikiServices::getInstance()->getDBLoadBalancer()->getConnection( DB_PRIMARY ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using dependency injection instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do with the rewrite.
| try { | ||
| $database->commit( | ||
| __METHOD__, | ||
| IDatabase::FLUSHING_INTERNAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is FLUSHING_INTERNAL used instead of FLUSHING_ONE?
Modules/Handlers/semantic_ask.php
Outdated
| "format" => "json", | ||
| "query" => $query | ||
| ] ); | ||
| $data = $api->apiPost( $postdata, true ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does $api come from?
Modules/Handlers/semantic_ask.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when a user visits this file directly from the browser? Why isn't this a proper API module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first comment was : TODO: Rewrite this to be an proper API call for version 2.7.0. This has been done
| return json_encode( | ||
| $ret, | ||
| JSON_PRETTY_PRINT | ||
| ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dead code
| if ( $wgScriptPath !== "" ) { | ||
| $smwQueryUrl = "/" . ltrim( $wgScriptPath, '/' ) . '/index.php/Special:FlexForm'; | ||
| $smwQueryUrl = "/" . ltrim( $wgScriptPath, '/' ) . '/api.php'; | ||
| } else { | ||
| $smwQueryUrl = '/index.php/Special:FlexForm'; | ||
| $smwQueryUrl = '/api.php'; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just:
$smwQueryUrl = '/' . $wgScriptPath . 'api.php';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have seen to many variations in $wgScriptPath to trust it to start with "/" or not
Modules/Handlers/semantic_ask.php
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code looks very similar to the code in the SemanticAsk class.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SemanticAsk classes will be removed
…l API call in 2.7.x