Page MenuHomePhabricator
Create Task
Maniphest T183072

SocialProfile - Provide a safe fallback for when user doesn't choose any avatar and presses "upload" on Special:UploadAvatar
Closed, ResolvedPublic
Actions

Description

Main environment:

Reproduction steps:

  1. Visit Special:UploadAvatar
  2. Don't press Choose file, go straight to pressing Upload file
  3. Receive PHP notices similar to this:
[116d670e43e48d4a8cf870b1] /wiki/Special:UploadAvatar BadMethodCallException from line 356 of /vagrant/mediawiki/includes/specials/SpecialUpload.php: Call to a member function setSubmitText() on a non-object (string)

Backtrace:

#0 /vagrant/mediawiki/includes/specials/SpecialUpload.php(571): SpecialUpload->showRecoverableUploadError(string)
#1 /vagrant/mediawiki/includes/specials/SpecialUpload.php(207): SpecialUpload->processUpload()
#2 /vagrant/mediawiki/extensions/SocialProfile/UserProfile/SpecialUploadAvatar.php(52): SpecialUpload->execute(NULL)
#3 /vagrant/mediawiki/includes/specialpage/SpecialPage.php(522): SpecialUploadAvatar->execute(NULL)
#4 /vagrant/mediawiki/includes/specialpage/SpecialPageFactory.php(578): SpecialPage->run(NULL)
#5 /vagrant/mediawiki/includes/MediaWiki.php(287): SpecialPageFactory::executePath(Title, RequestContext)
#6 /vagrant/mediawiki/includes/MediaWiki.php(851): MediaWiki->performRequest()
#7 /vagrant/mediawiki/includes/MediaWiki.php(523): MediaWiki->main()
#8 /vagrant/mediawiki/index.php(43): MediaWiki->run()
#9 /var/www/w/index.php(5): include(string)
#10 {main}

You might also see this warning (reproduced on local environment, latest version of REL1_30 on PHP 5.6.28):

Warning: getimagesize(): Filename cannot be empty in C:\xampp\htdocs\core\extensions\SocialProfile\UserProfile\SpecialUploadAvatar.php on line 361

Currently, SocialProfile doesn't handle this special situation correctly. There should be a safer fallback to handle this, such as a message saying "You have not chosen an avatar yet; please choose one before uploading".

Related Objects

Event Timeline

SamanthaNguyen created this task.Dec 17 2017, 1:23 AM
SamanthaNguyen moved this task from Backlog to Bugs on the SocialProfile board.
ashley triaged this task as Medium priority.Dec 17 2017, 2:52 PM
ashley added subscribers: Bawolff, matmarex.

Well ain't this a fun little bug.

To avoid excess code duplication etc. the Special:UploadAvatar reuses core Special:Upload's internals where possible and appropriate (as witnessed by the backtrace here, for example). However, SpecialUpload#getUploadForm returns an UploadForm instance which is a subclass of HTMLForm, whereas our SpecialUploadAvatar#getUploadForm returns a string. So when you hit the "upload" button on Special:UploadAvatar you wind up hitting SpecialUpload#showRecoverableUploadError which calls SpecialUploadAvatar#getUploadForm assuming that returns a(n) HTMLForm object on which it can call the setSubmitText method; but as we know, this is not the case.

What I find extremely fascinating is that SpecialUpload#showUploadForm, which takes a single parameter ($form), can handle the parameter being either a string or a(n) HTMLForm. So why don't SpecialUpload#showRecoverableUploadError and SpecialUpload#showUploadWarning check if ( $form instanceof HTMLForm ) before attempting to call setSubmitText (and in the case of SpecialUpload#showUploadWarning, addButton too) on the return value of SpecialUpload#getUploadForm?

While we can easily enough fix or at least work around this in SocialProfile with some code duplication, it seems to me that the proper long-term fix here would be to tweak core so that it can consistently handle the case of getUploadForm returning something else than a HTMLForm subclass.

cc'ing @Bawolff and @matmarex for thoughts on this.

matmarex added a comment.Dec 18 2017, 7:13 AM
In T183072#3843165, @ashley wrote:

What I find extremely fascinating is that SpecialUpload#showUploadForm, which takes a single parameter ($form), can handle the parameter being either a string or a(n) HTMLForm. So why don't SpecialUpload#showRecoverableUploadError and SpecialUpload#showUploadWarning check if ( $form instanceof HTMLForm ) before attempting to call setSubmitText (and in the case of SpecialUpload#showUploadWarning, addButton too) on the return value of SpecialUpload#getUploadForm?

That check in #showUploadForm was added in 98c968c7f7cb5279fb72353861126d8494fc8ad0: "Allow string to be passed to showUploadForm to make porting old extensions easier". So already in 2010, this was a workaround to support "old" extensions. Presumably no one looked at that function when implementing other stuff in the years since then, since #getUploadForm is documented to return an UploadForm (and it's not stated anywhere that subclasses can return strings).

While we can easily enough fix or at least work around this in SocialProfile with some code duplication, it seems to me that the proper long-term fix here would be to tweak core so that it can consistently handle the case of getUploadForm returning something else than a HTMLForm subclass.

If anything, fixing this in SocialProfile would reduce your code duplication. Like this chunk I see there that would become unnecessary:

		// The following two lines are delicious copypasta from HTMLForm.php,
		// function getHiddenFields() and they are required; wpEditToken is, as
		// of MediaWiki 1.19, checked _unconditionally_ in
		// SpecialUpload::loadRequest() and having the hidden title doesn't
		// hurt either
		// @see https://phabricator.wikimedia.org/T32953
		$output .= Html::hidden( 'wpEditToken', $this->getUser()->getEditToken(), array( 'id' => 'wpEditToken' ) ) . "\n";
		$output .= Html::hidden( 'title', $this->getPageTitle()->getPrefixedText() ) . "\n";
SamanthaNguyen unsubscribed.Jun 30 2018, 8:39 PM
ashley moved this task from Backlog to SocialProfile on the Social-Tools board.Jan 13 2020, 11:56 PM
ashley closed this task as Resolved.Jun 2 2024, 9:26 PM
ashley claimed this task.

Something somewhere has changed enough that this isn't happening anymore as far as I'm able to tell (on MW 1.39).

SocialProfile hides the upload button now by default until the user has chosen a file (or inputted a URL if they have the upload_by_url user right) since rESPRa0e70291b0e16d318284a90135f5a56d05b70e08 (due to T159623); if someone manually overrides the CSS to show the button and presses it without choosing a file, they get the "Empty file" error message displayed normally. If they override the CSS and have chosen the upload-by-URL option, the error message is "Invalid URL: " (which isn't the prettiest message but it does the trick).

Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits