Page MenuHomePhabricator

Review SignWriting MediaWiki Plugin extension code and commit it to SVN
Closed, DeclinedPublic

Description

Author: mike.lifeguard+bugs

Description:
What it says on the tin. We want to deploy this for testing on incubator, so the extension needs to be reviewed, and should be committed to version control. You might also consider giving Steve commit access to maintain it (http://www.mediawiki.org/wiki/User:Slevin).


Version: unspecified
Severity: enhancement
URL: http://www.signpuddle.net/mediawiki/extensions/swmp.zip

Details

Reference
bz22215

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 10:52 PM
bzimport set Reference to bz22215.
bzimport added a subscriber: Unknown Object (MLST).

mike.lifeguard+bugs wrote:

Added download link to URL field

Extension documentation at http://www.mediawiki.org/wiki/Extension:SignWriting_MediaWiki_Plugin

Added need-review keyword

Didn't add Steve to CC - does he have a bugzilla account?

Dunno if you actually need an account?

Also, why not just get him to apply for access, commit it, and wait for it to be reviewed? Not sure why it needs a bug..?

(In reply to comment #2)

Dunno if you actually need an account?

Yes. I did a search of the userlist but I can't find him to add.

Also, why not just get him to apply for access, commit it, and wait for it to
be reviewed? Not sure why it needs a bug..?

Agreed. Could repurpose this bug into "review extension and deploy to Incubator," but the actual "put it into SVN" doesn't need a bug.

[19:02:29] <Reedy> Size: 6.83 MB (7,164,781 bytes)
[19:02:39] <Reedy> Size on disk: 148 MB (156,164,096 bytes)

on NTFS..

Seems they do, yeah. Suppose would be worth poking him to join bugzilla (and mediawiki if he's not already).

Can then get a request for SVN.. If it's a priority, i'm sure Tim can be poked

happy.melon.wiki wrote:

Step 1: commit to SVN
Step 2: clean up code to our [[mw:Coding conventions]] and standards. This can either be in the form of someone getting commit access to maintain it, or through incessant bugzilla patch-and-poke.
Step 3: get security review from Tim
Step 4: deal with Tim's response
Step 5: goto step 3 until all-clear is given
Step 6: submit shell request

As for Slevin wanting commit access, he can request it on wiki in the mean time, which means it might be done before the bug is seen.

[1]. http://www.mediawiki.org/wiki/Commit_access_requests

slevin wrote:

I am user Slevinski:
http://www.mediawiki.org/wiki/User:Slevinski

I have my own private SVN and I'd rather not use another repository. If we can talk about installing the plugin without adding to SVN, let's close this bug and focus on 22216:
https://bugzilla.wikimedia.org/show_bug.cgi?id=22216

Complying with the coding conventions will need to be done before moving to the live production server, but what about the MediaWiki Incubator? This will take time I'd rather focus elsewhere:
http://www.signpuddle.net

If the coding conventions are required to move forward, I'll probably deploy a incubator project on my own server and we can revisit WikiMedia late 2010, early 2011. By then I should have the localization finished (or at least workable) and I can take time to refactor the code.

Please forgive my ignorance of WikiMedia internals. My plugin is not MediaWiki focused, but a specific implementation of the general SignWriting Image Server.

Ultimately, I'll solve this issue on the desktop. The plugin is a bridging strategy until desktop integration. I'd appreciate help getting there with this next step.

happy.melon.wiki wrote:

All Wikimedia wikis run from the same server cluster on the same LAMP stack; all code deployed on the cluster must be security reviewed. The MediaWiki installation on the cluster is pushed from the Wikimedia SVN repository; consequently all production code must be checked into SVN. You're welcome to work on any repository anywhere you like, but any code that you want reviewed or run must be checked in.

slevin wrote:

Thanks for the information. I'll strip the editor, refactor the viewer according to MediaWiki Coding conventions, and use the MediaWiki SVN.

A security review should be straight forward.

I'd like to start using the MediaWiki SVN in March. Who do I ask for access? I emailed Tim and I'll add my name to the Commit access requests page on Monday.

(In reply to comment #9)
Just wait, and it will be done and you will be notified at the appropriate time.

sumanah wrote:

Hi! I saw on https://www.mediawiki.org/wiki/Extension:SignWriting_MediaWiki_Plugin that your extension is not available in the MediaWiki SVN repository. Just an update that we have a new procedure for developers who want access to commit their extensions to Subversion, in case you'd like to do that.

https://www.mediawiki.org/wiki/Commit_access_requests#Requesting_commit_access

(In reply to comment #4)

[19:02:29] <Reedy> Size: 6.83 MB (7,164,781 bytes)
[19:02:39] <Reedy> Size on disk: 148 MB (156,164,096 bytes)

on NTFS..

I get 28 MB vs 171 MB (it has grown?). And 15 MB in the zip.

The file footprint could be much smaller (~96:1) if the rotation, flipping and filling was done by the script. That would appeal small sites. Large ones would still want to cache it in the fs.

The server code seems very messy, including html injection, register globals problems and ugly direct $_REQUEST manipulation (I understand you may want to keep it separate from, but you should avoid the notices if they are unset, eg. array addition to some defaults). There's a general lack of indentation, and at least on swmp.php should be tab based.

Old conventions MW extension uses: Registration should be done to the passed $parser with ParserFirstCallInit, not to $wgParser with $wgExtensionFunctions. Should use $wgExtensionAssetsPath

You should have very good reasons to defend this line:
$parser->disableCache();//should solve caching problem.
Luckily, it is apparently unneeded, so it seems it could be removed without harm.

The chdir() 'quick hack so scripts works' should be removed if possible. You can use DIR or dirname( FILE ) in the server includes (also not that include_path is not duaranteed to contain .).
Classes should be autoloaded.
There are also html injection problems in swmp.php but in general, shouldn't be hard to fix that file. I'm more concerned about making the server safe.

As it is now, there's no chance it gets enabled in any WMF project.

rm patch-need-review, reviewed in Comment 12. Please let me know if you need help fixing problems outlined there.

Bumping this bug. This bug's summary is currently "Review SignWriting MediaWiki Plugin extension code and commit it to SVN". Are new extensions still being added to SVN or are new extensions going into Git now?

(In reply to comment #14)
git.

(In reply to comment #14)

Bumping this bug. This bug's summary is currently "Review SignWriting MediaWiki
Plugin extension code and commit it to SVN". Are new extensions still being
added to SVN or are new extensions going into Git now?

Well, technically, one can still commit it to SVN, however this doesn't make it closer to deployment:) I think this bug could be WONTFIXed until the code is actually fixed, and since the extension just uses a non MediaWiki-specific sign writing library - it should be done by the library's author, unless there's a pressing need to fork.

I created the git repo about 2.5 weeks ago, and it looks like work's been done since then: https://gerrit.wikimedia.org/r/gitweb?p=mediawiki/extensions/SignWritingMediaWikiPlugin.git

slevin wrote:

I've addressed the issues that have been brought up. I will mark this issue as "resolved - won't fix" because the code is in GIT rather than SVN.