Page MenuHomePhabricator

Improving static analysis tools for MediaWiki
Closed, ResolvedPublic

Description

We currently have a PHPCS standard that MediaWiki core and a decent number of extensions are using.. We've seen the value of standardizing our code and making sure it is enforced in an automated manner. We are relying less on humans to worry about style and let them focus on the actual code.

That said, we can make phpcs more useful by adding more MediaWiki-specific sniffs. Some quick ideas I had:

  • Usage of $dbr->query() directly instead of the $dbr->select() wrapper
  • Using wfMessage() when $this->msg() is usable
  • Using globals ($wgUser, $wgRequest) when their context equivalents could be used instead ($this->getUser(), $this->getRequest())
  • Modifying certain globals ($wgUser, $wgResourceModules, etc) inside a $wgExtensionFunction where it is either too early or late to do so

I'm sure we can easily come up with a longer list based on people's code review checklists. In addition, some of the structure tests (tests/phpunit/structure in core) should become sniffs instead.

Furthermore, we currently really only use the check functionality of phpcs - finding errors. phpcs also supports automatically fixing errors through phpcbf. We should make sure that as many of our error finding sniffs as possible can be automatically fixed (as is reasonable), and that we have proper test coverage for that functionality.

We use PHP CodeSniffer and a custom standard in mediawiki/tools/codesniffer.git. It is versioned and published on packagist as mediawiki/mediawiki-codesniffer.

Addshore has done some initial work in enhancing the standard for MW core compatability (https://gerrit.wikimedia.org/r/153399), which would be a good place to start.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

I wonder if @polybuildr would be interested in mentoring?

I would be willing to mentor this project

I would be willing to mentor this project

Awesome ! Thank you @EBernhardson for taking up this one! Thats one more feather to Featured list in Possible-Tech-Projects !

I am interested in working for this project for the Outreacyh 11. As I understood from the task description as given to improve the codesniffer extension by adding Mediawiki specific sniffs sounds interesting. I have fixed few bugs and hoping that would help me do this project.

I would be willing to mentor this project

Awesome! :)

I am shifting this to #Outreachy-11 as the project description has two mentors, micro-tasks and looks ready for the 11th edition of Outreachy ( Dec 2015 - Mar 2016 ) . Potential candidates should start by submitting their proposals as a blocker for this task, by November 02.

Feel free to revert it back, if this task has some relevant issues which might block its completion in this term of Outreachy.

Sumit added a comment.Feb 19 2016, 8:16 PM
NOTE: Outreachy round 12 applications are now open and GSoC 2016 is round the corner. This project was featured for Outreachy round 11 and has a well defined scope. Are you ready to mentor the project this season? If yes, then we'll feature this for Outreachy round 12 and GSoC 2016 as well. Please reply back in comments.

In addition to phpcs, which is a frankly very minor start, I would really love to see this expand to running https://github.com/etsy/phan as part of the CI infrastructure. This may require patches both to phan and to mediawiki to get everything running well and usefull for use as a CI voter.

phan looks like pretty cool thing. One note though - while it does not require code being checked to be PHP 7, the tool itself needs PHP 7 + php-ast extension to run. Which means we'd have to get PHP 7 running on CI. Not sure if it will work on HHVM (given dependency on non-core extension, probably not OOB, though not sure if the delta is huge or not).

Niharika removed a subscriber: Niharika.Feb 20 2016, 5:21 AM
Sumit added a comment.Feb 23 2016, 4:53 PM

In addition to phpcs, which is a frankly very minor start, I would really love to see this expand to running https://github.com/etsy/phan as part of the CI infrastructure. This may require patches both to phan and to mediawiki to get everything running well and usefull for use as a CI voter.

@EBernhardson , do you think the above could be added in the current task for a 3 month Outreachy/GSoC internship?

Qgil removed a subscriber: Qgil.Feb 24 2016, 10:49 AM

Hello,

I am a student who are willing to join your CodeSniffer improvement project in the GSoC 2016. I have read the project source code from the Git. Adding CI support is a good idea, is there any other requirements besides this one?

In addition to phpcs, which is a frankly very minor start, I would really love to see this expand to running https://github.com/etsy/phan as part of the CI infrastructure. This may require patches both to phan and to mediawiki to get everything running well and usefull for use as a CI voter.

@EBernhardson , do you think the above could be added in the current task for a 3 month Outreachy/GSoC internship?

I have a hard time estimating what an intern could or could not accomplish in 3 months. There is still a big question around them being able to work in a wide variety of cases, from getting php7 and php-ast installed on the CI hosts, working with Jenkins configuration, and working up tooling around phan to deal with our code likely being in a mediocre state wrt following the types our annotations claim.

I think i would rather mark it as a stretch goal, or perhaps an independent project if someone is interested in taking it on rather than risk making this initial project too big.

Hello,

I am a student who are willing to join your CodeSniffer improvement project in the GSoC 2016. I have read the project source code from the Git. Adding CI support is a good idea, is there any other requirements besides this one?

My reading is that CI support already exists, it is just not voting yet because the code is not in a sufficient state. Being able to make it voting for a wide variety of projects, IMO, would mean working up something that knows the state of the repository prior to the patch and rather than voting that all things passed votes that no new regressions exist. I think this would be the best course of action because it would allow phpcs to be made voting everywhere rather than needing to get a hundred repositories into a state where they completely pass.

There is also the matter of adding new sniffs that enforce conventions that are not yet codified I. Phpcs.

This task needs a few updates. PHPCS is now voting in several repositories (including core, iirc).

Hi,

I'm a student who are willing to take part in GSoC 2016. I have read the current CodeSniffer code, and I'm interested in the CI ,I hope I can do something for it.

Hello,

I noticed that the procedure for checking dirname() is crowded with too many if statements. Perhaps we can refactor it into a loop? and it seems a little confused , especially new features need. refactor the struct of CodeSniffer is helpful?

By reading source code and document I noticed that setup functions that are registered to the $wgExtensionFunction should not read some global properties too early, but what about "too late"? I don't quite get that.

Sumit added a comment.Mar 3 2016, 5:46 PM

This task needs a few updates. PHPCS is now voting in several repositories (including core, iirc).

@Legoktm @polybuildr can you update the task description to reflect the changes that phpcs has undergone and the todo's left with respect to this task?

lucky added a comment.Mar 3 2016, 9:15 PM

Sir,
I am Interested to work on the project:Implement HTML e-mail support in
MediaWiki.
What are the things i need to do next?

@lucky: That is off-topic for this task and not handled here. Please also see Answering your questions - thanks!

Legoktm updated the task description. (Show Details)Mar 4 2016, 9:19 AM

I updated the task description. @Addshore, @EBernhardson, are you both still up for mentoring this? (I have another project or two I'd like to mentor ideally...) :)

Hello,
In the past few days, I add some features about the MediaWiki CodeSniffer, it's useful? @EBernhardson @Addshore @Legoktm

  • make sure a space between closing parenthesis and opening brace
  • during if,while,for,foreach,switch,catch statement
  • fail: if ( $a == 1 ){
  • fail: if ( $a == 1 )\eol\t{
  • fail: switch ( $a ){
  • pass: if ( $a == 1 ) {
  • pass: switch ( $a ) {

I has completed the code of this sniff, and what should I do to push request?

@Lethexie: Patches in Gerrit against the mediawiki/tools/codesniffer repository are welcome.
Please see Developer access (and How to become a MediaWiki hacker for the broader picture).

Hello! I am interested in working on this. By when will microtasks be posted for this project?

@Aklapper : Thank you, I have submitted my changes to Gerrit.

@Lethexie: Please follow https://www.mediawiki.org/wiki/Gerrit/Commit_message_guidelines and add a "Bug: T89682" to your commit message, so there will be an automatic link in this task. Cannot find your patch in Gerrit.

Change 275747 had a related patch set uploaded (by Lethexie):
Add the SpaceBeforeControlStructureBraceSniff

https://gerrit.wikimedia.org/r/275747

Change 275796 had a related patch set uploaded (by Lethexie):
Changes to SpaceyParenthesisSniff

https://gerrit.wikimedia.org/r/275796

Change 276110 had a related patch set uploaded (by Lethexie):
Changes in SpaceAfterControlStructureSniff.php

https://gerrit.wikimedia.org/r/276110

Change 277179 had a related patch set uploaded (by Billm):
Apply setting variable instead of direct warning value

https://gerrit.wikimedia.org/r/277179

Change 277227 had a related patch set uploaded (by Lethexie):
Fix IfElseStructureSniff can't detect and fix multiple white spaces after else

https://gerrit.wikimedia.org/r/277227

Change 277557 had a related patch set uploaded (by Billm):
Add ability for IfElseStructureSniff to detect multiple white spaces after else

https://gerrit.wikimedia.org/r/277557

Hello

I am interested in working on this project for this upcoming GSoC and I want to know which other MediaWiki-specific sniffs are to be worked on this project.

@Lethexie @Billghost Please dont associate your patchsets to microtasks with the main project task. This would make the main one have a Patch-For-Review, which is not desirable. Thank you!

Ok @01tonythomas, sorry for the confusion

Please associate your proposal with the correct project tags. Please take a look at Life_of_a_successful_project#Submitting_your_proposal and remove the unwanted tags like Possible-Tech-Projects

@Legoktm @EBernhardson : Could you provide some microtasks to work on? Thanks!

Addshore updated the task description. (Show Details)Mar 22 2016, 9:13 AM

Change 277179 had a related patch set uploaded (by Billm):
Apply setting variable instead of direct warning value

https://gerrit.wikimedia.org/r/277179

@Billghost : Yet again, please dont tag your commits with unrelated change. Please take a thorough look at www.mediawiki.org/wiki/Gerrit/Commit_message_guidelines before pushing in next change.

T130574 (the proposal for this task) got resolved and this is listed on https://www.mediawiki.org/wiki/Google_Summer_of_Code/Past_projects#Improving_static_analysis_tools_for_MediaWiki - what is left to do in this task?

There are two open sub tasks but they do not seem to actually be subtasks at all but follow-ups instead?

EBernhardson closed this task as Resolved.Sep 17 2018, 4:45 PM
EBernhardson claimed this task.

Task is completed, the extra analysis checks have proven themselves useful in a variety of circumstances.