right now, the user needs to edit settings.php (or worse, the .module itself) to configure the wiki into Drupal. That should be in admin/ somewhere and configured through variables (including the secret key).

CommentFileSizeAuthor
#7 mediawiki_db_config_and_uninstall.patch7.14 KBseanburlington
#6 mediawiki_db_config.patch6.28 KBseanburlington
#3 mediawikiauth.module.patch13.65 KBseanburlington

Comments

thinkling’s picture

thinkling commented

When doing this, remember the retain the ability to have multiple wikis associated with one Drupal login front-end. Several people who use the code do this.

anarcat’s picture

anarcat commented
Priority: Critical » Normal
seanburlington’s picture

seanburlington commented
Status: Active » Needs work
StatusFileSize
new mediawikiauth.module.patch13.65 KB

Hi,
the attached patch enables the configuration of the module to be stored in the Drupal database with an admin settings page for configuration.

It also makes the https only setting configurable rather than auto detected - the reason for this is that I am using securepages for login on https - but want to redirect users to http for pages that are not sensitive.

It should work for multiple wikis - but this is untested.

I am not sure it's 100% right as the cookies set my the wiki are not being accepted - but I'm seeing this on my live site too so I'd appreciate a review of this code from someone who maybe has a better understanding of the mediawiki module side of things.

The login and logout is successful - the DRXtrArgs and DRXtrArgs2 are working - but the setCookie calls in AuthDrupal don't seem to work (I need to check if there's an update available)

seanburlington’s picture

seanburlington commented
Status: Needs work » Needs review

I figured out that the mediawiki cookies weren't being set because I was using a development domain with no . in it

as a security measure browsers won't accept cookies for these sort of domain - to prevent people setting cookies for all com or all uk (etc) domains.

Once I moved from http://dev to http://dev.localhost the cookies started working as expected.

So I think the code attached in the previous comment is good and successfully moves the wiki settings into the database

anarcat’s picture

anarcat commented
Status: Needs review » Needs work

First, thanks for the patch. It's great to see efforts in this direction.

I'm not sure I'm ready to accept the patch at this point. It seems to be too far-ranging to resolve this specific problem and only that problem. For example, in the patch there's a completely new mediawikiauth_encode() function. I understand that this probably replaces the authdrupal_encode() function, but then why is the latter still in AuthDrupalEncode.php? We should at least remove the duplication. In fact, i would rather see the rewrite of that function in a separate patch (which I would still welcome, as that code is messy right now). (Same applies for StaticUserLogout(), by the way.)

I also feel that mediawikiauth_get_vals() is a misnomer: this name doesn't mean much and we need to read the function to understand what it does. Maybe mediawiki_get_configs()?

Also, there are a lot of comments changes in the code: I'm afraid that we loose documentation on how this connects with mediawiki. Maybe I'm being too paranoid here. There are also whitespace changes in the patch I would rather not see.

Finally, since you are introducing values in the variable table, you need to clean them up on uninstall. So you need also to patch the .install file.

So in short: keep topically changes in separate patches (whitespace, function rewrites), write a uninstall hook, don't duplicate code and rename mediawikiauth_get_vals().

How does that sound?

Thanks again,

seanburlington’s picture

seanburlington commented
Status: Needs work » Needs review
StatusFileSize
new mediawiki_db_config.patch6.28 KB

I've re-rolled the patch against the latest CVS code

This is now a minimal patch leaving all variables as globals but loading from database.

This has reduced the number of lines edited a lot and means that the external functions will work as is.

This use of globals is not very Drupal - but happy to provide an easier to review patch.

I haven't tested the code properly yet as it no longer works for my project - I'll do that at the weekend.

seanburlington’s picture

seanburlington commented
Category: feature » bug
StatusFileSize
new mediawiki_db_config_and_uninstall.patch7.14 KB

Hi,
I've now done a reasonable bit of testing on this code and am happy that it works as intended

It also now includes an uninstall hook so I hope it meets all the points you raised.

It seemed that the most helpful thing to do would be to work against the latest CVS version - but I did find a couple of bugs there - in order to keep this patch minimal it doesn't include those fixes which are both needed for this patch to work properly on the CVS version.

#928524: syntax error in CVS version - mediawikiauth_StaticUserLogout() line 85
and
#932590: StaticUserLogout sets wong cookieprefix in CVS version

seanburlington’s picture

seanburlington commented
Category: bug » feature

oops accidental category change

anarcat’s picture

anarcat commented
Status: Needs review » Reviewed & tested by the community

This is an excellent patch, just how I like it: simple and easy to review, it brings a proper and legitimate incremental change. :)

I agree that globals are retarded, but the issue here is not globals but where the configuration is stored. We can fix the globals issue later.. and I would really like it if you could finish that effort.

That can go in now.

dhigby’s picture

dhigby commented

Thanks for a great module. I would appreciate an updated release with the patches if you can manage it. What about a dev version? I don't enjoy patching. I do it so rarely there is a learning curve each time.

mshaver’s picture

mshaver commented

How do these patches related to the module code included with the extension at:

http://thinkling.com/pub/src/AuthDrupal/AuthDrupal-0.7.1.tgz

thinkling’s picture

thinkling commented

@mshaver: Not to start a longer discussion in this thread, but--right now the code here is a fork of the code linked at mediawiki.org. If & when it looks like the code here is really moving forward, I'll submit my patches for changes made over there since the fork.

seanburlington’s picture

seanburlington commented
Assigned: Unassigned » seanburlington
Status: Reviewed & tested by the community » Needs work

Well it's taken a while (sorry folks) but I've committed the patch in #7

I think this needs more testing before releasing even a dev version - and certainly some documentation - so I'm marking needs work for now

Before upgrading - backup your original module as it will contain your settings which now need to be configured at admin/settings/mediawikiauth