Last modified: 2013-10-23 18:17:13 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T34760, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 32760 - Installer should check for old StartProfiler.php files
Installer should check for old StartProfiler.php files
Status: REOPENED
Product: MediaWiki
Classification: Unclassified
Installer (Other open bugs)
1.18.x
All All
: Normal normal (vote)
: Future release
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-12-02 00:13 UTC by Mark A. Hershberger
Modified: 2013-10-23 18:17 UTC (History)
6 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Mark A. Hershberger 2011-12-02 00:13:47 UTC
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...
Comment 1 Daniel Friesen 2011-12-02 01:36:05 UTC
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)
Comment 2 Chad H. 2011-12-02 02:08:56 UTC
(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.
Comment 3 Daniel Friesen 2011-12-02 02:27:20 UTC
(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.
Comment 4 Gordon Joly 2011-12-07 11:08:18 UTC
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?
Comment 5 Daniel Friesen 2011-12-07 15:46:07 UTC
(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.
Comment 6 Krinkle 2012-03-02 21:19:15 UTC
Moving to 1.19.0 as 1.18 is released.
Comment 7 Mark A. Hershberger 2012-10-01 17:31:52 UTC
updating milestones
Comment 8 Mark A. Hershberger 2013-01-17 17:29:19 UTC
https://www.mediawiki.org/wiki/Thread:Project:Support_desk/php_update.php_error

Another instance.
Comment 9 Chad H. 2013-01-17 17:36:57 UTC
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.
Comment 10 Mark A. Hershberger 2013-01-19 20:40:17 UTC
(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?
Comment 11 Chad H. 2013-01-19 23:35:01 UTC
(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...
Comment 12 Mark A. Hershberger 2013-01-20 01:53:13 UTC
(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?

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links