Page MenuHomePhabricator

Installer should check for old StartProfiler.php files
Closed, DeclinedPublic

Description

from http://permalink.gmane.org/gmane.org.wikimedia.mediawiki/38411

I have Mediawiki installation went from 1.17 through 1.18 RC1 and to 
1.18 - no problems.

But another Mediawiki has had some problems going direct from 1.17 to 
1.18 - I took out Recaptcha and now see this:


*Fatal error*: Cannot redeclare wfprofilein() (previously declared in 
/xxxxxxxx/public_html/includes/profiler/Profiler.php:14) in 
*/xxxxx/public_html/includes/ProfilerStub.php* on line *25*

Not sure why...

Version: 1.18.x
Severity: normal

Details

Reference
bz32760

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 12:07 AM
bzimport set Reference to bz32760.
bzimport added a subscriber: Unknown Object (MLST).

My response to that was:
"""
Sounds like you upgraded MediaWiki by extracting 1.18 on top of 1.17
instead of extracting 1.18 to a new directory then moving the config and
other stuff to the new directory. And have an old StartProfiler.php.

includes/ProfilerStub.php was moved to includes/profiler/Profiler.php and
includes/profiler/ProfilerStub.php in 1.18

Basically what it looks like is happening is that you have a
includes/ProfilerStub.php file from 1.17 and includes/profiler/ from 1.18.
Because your StartProfiler.php likely uses the old method of a
require_once (instead of autoloading as I believe we now do) MediaWiki is
loading the 1.18 profiler code from includes/profiler/Profiler.php and
then when it require your StartProfiler.php that old require_once is
requiring the old 1.17 code from includes/ProfilerStub.php causing a fatal
error as the old code tries to redefine the same method.

If you don't need the profiler I recommend you delete your
StartProfiler.php. If you do you should probably delete the require_once
line.
You'd also probably be better off clearing out all the outdated files of
1.17 code that were left behind since you extracted 1.18 over top of 1.17.
Though the easiest way to do that is to just re-untar 1.18 in another
directory, and move your actual LocalSettings, extensions, images/, etc...
to there.
"""

There are probably a few things we can do:

  • Update the updating docs with more instructions on how to update MediaWiki by creating a new directory, and make strong notes on how extracting MW over MW is a bad idea.
  • Update the profiler docs so it stops using outdated docs that use require_once lines and properly use the autoloader instead.
  • In the next 1.18 release include a includes/ProfilerStub.php that contains something like <?php require_once( "$IP/includes/profiler/ProfilerStub.php" );. Though, to be honest we could probably get away with a completely empty file or just <?php with a comment on why the file exists. (This would fix the issue by overwriting the 1.17 files and providing a no-op file so that the require_once's don't fail)

(In reply to comment #1)

  • Update the profiler docs so it stops using outdated docs that use

require_once lines and properly use the autoloader instead.

Profiler already uses AutoLoader and no require()s are necessary. StartProfiler.sample *was* updated when I refactored all of this.

  • In the next 1.18 release include a includes/ProfilerStub.php that contains

something like `<?php require_once( "$IP/includes/profiler/ProfilerStub.php"
);`. Though, to be honest we could probably get away with a completely empty
file or just <?php with a comment on why the file exists. (This would fix the
issue by overwriting the 1.17 files and providing a no-op file so that the
require_once's don't fail)

This is a good idea.

(In reply to comment #2)

(In reply to comment #1)

  • Update the profiler docs so it stops using outdated docs that use

require_once lines and properly use the autoloader instead.

Profiler already uses AutoLoader and no require()s are necessary.
StartProfiler.sample *was* updated when I refactored all of this.

Yeah, StartProfiler.sample is how I figured out that we autoload.

Our docs on the other hand will lead the many many people who don't read StartProfiler.sample into adding bad require_once lines which will fatal in 1.18 either due to not having the file or having a 1.17 copy that causes cryptic duplicate function fatals.
http://www.mediawiki.org/wiki/Profiling#Profiling

Now that I think about it, we should probably include a wfDeprecated( 'includes/ProfilerStub.php' ); into that no-op.

Yes, wfDeprecated('includes/ProfilerStub.php' ); is good.

But this issue remains - should the upgrade (from 1.x to 1.x+1) method be to unpack the tar image in place, or to move all files away (except LocalSettings.php and /images/ and /extensions/ etc) and copy in new versions into a blank directory?

(In reply to comment #4)

Yes, wfDeprecated('includes/ProfilerStub.php' ); is good.

But this issue remains - should the upgrade (from 1.x to 1.x+1) method be to
unpack the tar image in place, or to move all files away (except
LocalSettings.php and /images/ and /extensions/ etc) and copy in new versions
into a blank directory?

I don't think there's any debate over that by anyone but people who treat our badly written manual as a set of rules and don't actually read what it says.

Upgrades should be done by extracting to a new folder. This issue actually wouldn't have been fixed by doing that (though it would have been easier to figure out what was wrong) but extracting over top of your install is just plan bad planning. If something goes wrong it's not going to be easy to undo what you did and return to your backup. And you can inadvertently erase any modifications you've made.

Moving to 1.19.0 as 1.18 is released.

I really don't think it's worth it...it only affects people who have profiling enabled (not many--consider how few people have complained over the past 2+ years), and the docs have been updated for some time to reflect this.

On the other hand, adding any sorts of checks or back-compat adds code we have to support until who knows when.

Highly suggest WONTFIX.

(In reply to comment #9)

On the other hand, adding any sorts of checks or back-compat adds code we
have to support until who knows when.

One of the major benefits of the installer is that it can check your installation to make sure you don't have things that are going to cause problems later.

If someone does have a problem, they will end up doing one of the following:

  • Trashing MW -- "dang thing is impossible to upgrade, even with the new installer!"
  • Staying on their old version (similar to the above).
  • Asking for help from people who may or may not recognise their problem right away. If they do know, they're time is being taken up doing something the installer could have done. If they don't know, they'll end up very frustrated after searching a while.

I'm not asking you to fix this -- you already have a lot to do -- but when I provide an acceptable patch (this weekend?) could you merge it in?

(In reply to comment #10)

I'm not asking you to fix this -- you already have a lot to do -- but when I
provide an acceptable patch (this weekend?) could you merge it in?

I disagree that this needs fixing at all, I wasn't complaining about fixing it myself...

(In reply to comment #11)

I disagree that this needs fixing at all, I wasn't complaining about fixing
it myself...

I do understand that. I was just trying to preempt that concern.

Do you not see any validity to my argument? If I were to provide a fix for this or similar sanity checks in the installer, would you be willing to merge it?

Support for StartProfiler.php was removed in MW 1.32. Any old file on disk can no longer cause issues in the way described here.

https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/453893/