Skip to content
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

Update for new GraphViz extension class structure #365

Merged
merged 1 commit into from
Jan 6, 2018

Conversation

samwilson
Copy link
Contributor

@samwilson samwilson commented Jan 2, 2018

This PR is made in reference to: https://phabricator.wikimedia.org/T181926

This PR addresses or contains:

  • Adds extra check for whether the GraphViz extension is loaded.
  • Parses the graphviz source in a more future-compatible way (i.e. the <graphviz> element, rather than directly calling the render method).

This PR includes:

  • Update of RELEASE-NOTES.md
  • Tests (unit/integration)
  • CI build passed

@JeroenDeDauw
Copy link
Member

Is this compatible with MW 1.27?

Also pass the dot source to GraphViz in a slightly more robust
way (the args parameter was an empty string, but it should have
been an array).

Bug: T181926
@samwilson
Copy link
Contributor Author

Good point! I forgot about the parser service. Switched now; see what you think.

@@ -112,7 +115,7 @@ protected function getResultText( SMWQueryResult $res, $outputmode ) {
$graphInput .= "}";

// Calls graphvizParserHook function from MediaWiki GraphViz extension
$result = GraphViz::graphvizParserHook( $graphInput, "", $GLOBALS['wgParser'], "" );
$result = $wgParser->recursiveTagParse( "<graphviz>$graphInput</graphviz>" );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$GLOBALS['wgParser'] is better in the sense that you do not pollute the whole function scope with the global

Copy link
Member

@JeroenDeDauw JeroenDeDauw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mwjames
Copy link
Contributor

mwjames commented Jan 6, 2018

Does this change implies that GraphViz 3.+ is required?

@mwjames mwjames merged commit 7a577f4 into SemanticMediaWiki:master Jan 6, 2018
mwjames added a commit that referenced this pull request Jan 6, 2018
@samwilson samwilson deleted the graphviz branch January 6, 2018 09:04
@samwilson
Copy link
Contributor Author

@mwjames: Thanks! And no, it works for old or new versions of GraphViz.

@kghbln
Copy link
Member

kghbln commented Jan 6, 2018

Does this change implies that GraphViz 3.+ is required?

Thanks! And no, it works for old or new versions of GraphViz.

@samwilson @mwjames

FYI However more sysadmin work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants