r85288 MediaWiki - Code Review archive

Got a big of a whinge here...

class RequestContext {

	/**
	 * @var WebRequest
	 */
	private $mRequest;

class MediaWiki {

	/**
	 * TODO: fold $output, etc, into this
	 * @var RequestContext
	 */
	private $context;

	public function request( WebRequest $x = null ){
		return wfSetVar( $this->context->request, $x );
	}

	public function output( OutputPage $x = null ){
		return wfSetVar( $this->context->output, $x );
	}

We've got the usage of $output and $mOutput inside and outside of the context, and it looks messy

Same for $request and $mRequest

I don't think anything's actually wrong with that code in terms of variable access/visibility/whatever. The magic methods just add an exotic twist to working out what the best naming convention is...

I'm actually not so sure about this approach any more; in r86872 I wanted to make RequestContext itself an IContextSource, and not being able to do so offended my sense of recursion... :D Plus it's not so good for type hinting. I think just abstracting this away by reimplementing r86872 would be preferable.

Just seemingly having 2 references to possibly multiple objects of the same things seems a bit daft/wrong :)

Which is more confusing. I was looking at index.php and noticed both seemed to be used at a low level above it (in the RequestContext and the MediaWiki classes)

If you want me to find it specifically, let me know, but it'd be nice to tidy it up

ie I'm not saying the outside member access isn't ok (it's fine!), just the duplicate usage of them smells bad :)

You can't use __get() in PHP.

I tried it once before, but I had to self-revert because PHP's implementation of it is so broken. The issue is the re-entry guard, which will randomly break your code due to subtle changes in initialisation order. If you try to re-enter __get(), even with a different $name, PHP will stop the function from being called and will return null instead, without even raising a warning. So if one of the get*() functions happens to depend on another lazy-initialised member of RequestContext, the result is subtle breakage dependent on initialisation order.

It looks like quite a complex revert, so maybe I will leave this for a day or two and hope someone will do it for me.

I never knew __get() had this problem, yet I've used it successfully in another project. The important thing is that you can't use the magic behavior internally: all your getters and everything that could be called by __get() must use the real accessors instead of __get() / __set() magic. I guess this just kind of seemed like an obvious good idea to me, so I never ran into this bug.

The get functions in this revision call half the codebase: User, Language, Title, etc. You can't guarantee that nobody is ever going to use RequestContext from one of those modules. If you tried to use it in places where it was safe, that would establish a convention which would then be applied to places where it is not safe. It's better to just remove the temptation.

I've reverted in r90899 what bits of this haven't already been overwritten by later changes.