r69908 MediaWiki - Code Review archive

The parent of ForeignDBFile is LocalFile which is not the correct behavior. File:: was actually appropriate in this case. "Restore _remote_ behavior"

Reverting it fixes the problem. I wonder why it was changed, though. Nothing in his comment seems to indicate why.

Oh yeah and (sorry for the double comment), it shouldn't be inherited by LocalFile in the first place. Why go from a localfile directly to a foreign file. That is just asking for trouble.

Iirc, it wasn't a static method, so the calling way is wrong

The wrong inheritance tree isn't my fault though

Perhaps, but then your change was still a half-assed job. How very unGerman of you.

Before this comment turns into a discussion on here, I'd rather avoid to repeat myself, so I am pasting what I said to TheDJ on #mediawiki about the comment itself:

15:35:45 < thedj> ehm, is that really necessary ? 15:35:55 < svip> thedj: Slighty, yes. 15:36:15 < svip> Why make a change to an object to call its parents' functions

                without actually checking the inheritence.

15:36:27 < svip> Or verifying that the change did not mess things up? 15:36:49 < svip> I may be singled out here, but I always thought that verifying

                the changes you made before committing them was important.

15:37:31 < svip> Now, I understand that Reedy may not have had access to a

                FileRepo of that sort to verify it, then he should rather have 
                suggested the change to someone with the access to a repo to 
                verify that it did not break anything.

The better question is why these functions are being overloaded anyways. At least the getDescriptionUrl() can be removed from both LocalFile.php and ForeignDBFile.php (the repo's are supposed to provide the correct descriptionUrl, not the file classes)

For getDescriptionText(), i note that according to the comments in LocalFile, the function is never actually used for localfiles... So as such... does it belong at all in filerepo/ ? perhaps the upper level getDescriptionText should be moved to ImagePage, or be made a static helper function of File ?

Very true, I don't have the code setup to test all this.

As was discussed on another Rev, between I believe Platonides, Myself, Nikerabbit and Simetrical (Aryeh), testing these shouldn't have to be a manual process. I'm not pointing fingers here or anything, but Unit tests should exist, that have coverage of these things, so people not familiar with the area, can make changes, and have not made any obvious regressions. And then see from the tests failing that there are problems from the changes.

unGerman? I'm not sure what that's supposed to mean, not being German. Also, calling it "half-assed" is a little rude.

In a similar vain, shouldn't people be using the correct code constructs and accessors originally? (Again, not pointing fingers)

And as per TheDJ, it's systematic of other problems, and suggestion that there may be other solutions to it.

The other way, of course, would be finding the original revisions, or posting a bug saying "Can you fix this". Which, IMHO creates pointless work. Or something, but then, it might've just sat there and not been actioned. Being proactive, and attempting to fix the error, has brought it to light, and although (granted, I agree) it may have not be the correct fix, it highlighted the problem for people to see. Surely, this, among other reasons is the reason we have code review?

Phase3 isn't supposed to be stable(conversely, it shouldn't be majorly broken), is subject to breakages and changes

We could use the following in the getDescriptionText() of LocalFile. Not really proper either, but to duplicate the code or to split these classes will be a bit messy as well I think.

> if( $this instanceof ForeignDBFile ) { > return parent::getDescriptionText(); > }