r74035 MediaWiki - Code Review archive

This breaks compatibility with current 1.16 branch (again...). I would like that people using 1.16 be able to use the extension in trunk/, at least until Mw 1.17 is released (when you can safely tell people to upgrade MediaWiki before using the trunk version of the extension).

If I revert this, I'll have to add the GAID_FOR_UPDATE constant back that I removed in r74035, since doing one without the other would break these on trunk too.

There should be no problem to have both define() and the const. PHP (at least current 5.3.2) evaluate these two in different parser contexts.

Simple test:

<?php
define( 'GAID_FOR_UPDATE', 1 );
class Title {
	const GAID_FOR_UPDATE = 1;
};
print GAID_FOR_UPDATE . "\n";
print Title::GAID_FOR_UPDATE . "\n";

But the new replacement constant doesn't have to have the same name too. s/Title::GAID_FOR_UPDATE/Title::LATEST_REV_FOR_UPDATE/g (for example) should fix this problem.

I'm not saying it's a problem, I was just stating it would have to be done.

Back-compat be damned.

+1

A few months ago, r66934 had to be reverted for the very same reason. I thought it was accepted that it is desirable that extensions in trunk/ be compatible with the current stable release of MediaWiki (currently 1.16). Not a requirement, but at least an expectation for a good reason.

It didn't have to be reverted. It was reverted because I didn't want to argue it.

It could also be done in the extensions themselves: if version_compare ( $wgVersion , 1.17, >=) define('GAID_FOR_UPDATE', Title::GAID_FOR_UPDATE);

(with a !defined() so you don't create warnings by including several of these extensions)

I would support adding define('GAID_FOR_UPDATE', Title::GAID_FOR_UPDATE) for at least until 1.17 is released and possibly later.

+1

That would cause Title.php to be autoloaded when Defines.php is included though

I don't think that would be nice for installs without a bytecache. For instance, load.php would be loading that file for nothing. Also note, Defines.php is currently included before the autoloader.

This is a temporary hack for backwards compatibility. I don't think anyone intends to change that constant anytime soon. It should be fine to have the constant be doubly-defined until the old one gets removed after 1.17.

   define( 'GAID_FOR_UPDATE', 1 );

If necessary, leave a comment near it to warn careless passers-by that the value must be the same in Title::GAID_FOR_UPDATE.

IMO, more than that is to overthink the issue.

In fact, there wouldn't be such autoloading issue if the bc define was kept in Title.php

I just put it in defines.php

Except that it will break if you do not have Title.php autoloaded. It all depends on subtle differences in code flow that I would rather not rely on.

But that issue already existed in the previous version where the define was in Title.php. If it worked, it will continue working, if it's new it should be using Title::GAID_FOR_UPDATE anyway. I moved it in r75422

Ok.

Translate extension is broken for 1.16.

Fixed in r89696.