Last modified: 2014-04-08 23:10:22 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 T61788, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 59788 - Invalid or virtual namespace -1 given.
Invalid or virtual namespace -1 given.
Status: NEW
Product: MediaWiki extensions
Classification: Unclassified
WikidataRepo (Other open bugs)
unspecified
All All
: Normal normal with 1 vote (vote)
: ---
Assigned To: Wikidata bugs
:
: 59797 (view as bug list)
Depends on:
Blocks: 57026
  Show dependency treegraph
 
Reported: 2014-01-07 19:56 UTC by Sam Reed (reedy)
Modified: 2014-04-08 23:10 UTC (History)
6 users (show)

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


Attachments

Description Sam Reed (reedy) 2014-01-07 19:56:10 UTC
ApiQueryBase->2014-01-07 19:54:51 mw1071 wikidatawiki: [35dbb51c] /wiki/Special:NewItem   Exception from line 117 of /usr/local/apache/common-local/php-1.23wmf9/includes/WikiPage.php: Invalid or virtual namespace -1 given.
#0 /usr/local/apache/common-local/php-1.23wmf9/includes/context/RequestContext.php(200): WikiPage::factory(Object(Title))
#1 /usr/local/apache/common-local/php-1.23wmf9/extensions/SpamBlacklist/SpamBlacklistHooks.php(28): RequestContext->getWikiPage()
#2 [internal function]: SpamBlacklistHooks::filterMergedContent(Object(RequestContext), Object(Wikibase\ItemContent), Object(Status), '/* wbeditentity...', Object(User), false)
#3 /usr/local/apache/common-local/php-1.23wmf9/includes/Hooks.php(199): call_user_func_array('SpamBlacklistHo...', Array)
#4 /usr/local/apache/common-local/php-1.23wmf9/includes/GlobalFunctions.php(4032): Hooks::run('EditFilterMerge...', Array)
#5 /usr/local/apache/common-local/php-1.23wmf9/extensions/Wikibase/repo/includes/EditEntity.php(685): wfRunHooks('EditFilterMerge...', Array)
#6 /usr/local/apache/common-local/php-1.23wmf9/extensions/Wikibase/repo/includes/specials/SpecialNewEntity.php(103): Wikibase\EditEntity->attemptSave('/* wbeditentity...', 65, 'c04d679589cc8f2...')
#7 /usr/local/apache/common-local/php-1.23wmf9/includes/specialpage/SpecialPage.php(674): Wikibase\Repo\Specials\SpecialNewEntity->execute(NULL)
#8 /usr/local/apache/common-local/php-1.23wmf9/includes/SpecialPageFactory.php(488): SpecialPage->run(NULL)
#9 /usr/local/apache/common-local/php-1.23wmf9/includes/Wiki.php(298): SpecialPageFactory::executePath(Object(Title), Object(RequestContext))
#10 /usr/local/apache/common-local/php-1.23wmf9/includes/Wiki.php(596): MediaWiki->performRequest()
#11 /usr/local/apache/common-local/php-1.23wmf9/includes/Wiki.php(460): MediaWiki->main()
#12 /usr/local/apache/common-local/php-1.23wmf9/index.php(49): MediaWiki->run()
#13 /usr/local/apache/common-local/w/index.php(3): require('/usr/local/apac...')
#14 {main}
Comment 1 Sam Reed (reedy) 2014-01-07 21:25:08 UTC
*** Bug 59797 has been marked as a duplicate of this bug. ***
Comment 2 Brad Jorsch 2014-01-07 22:02:59 UTC
<anomie> aude, greg-g: Is this bug 59788? It looks like what's going on there is that Wikibase has been passing a context for a special page to the EditFilterMergedContent hook, and Gerrit change #101490 to SpamBlacklist made the reasonable assumption that the context for that hook would be for a page that's actually editable.
Comment 3 Aude 2014-01-07 22:53:49 UTC
for new entities via the special page (and api?), EditEntity passes the context of the special page (or who knows what) to SpamBlacklist.

Before I8c8b293, SpamBlacklist did:

$pout = $content->getParserOutput( $title );

with $title being Special:NewItem and $title never is used by EntityContent::getParserOutput so there was no effect.

That might not be so nice to give special page context / title, but at this point in EditEntity::attemptSave, the entity does not have an id yet. So we don't really know what the actual title would be yet and might not make sense to assign an id at that point.

Alternatively, perhaps $title is not really needed in SpamBlacklist for new content?

SpamBlacklist needs title for existing pages, but $title seems less necessary for new content. (it's used to make a log entry, afaik). 

For all content, a "flag" is also put on $title in case it's being checked more than once.

"if ( isset( $title->spamBlackListFiltered ) && $title->spamBlackListFiltered )..."

Need to poke around to see what an appropriate solution to this is.
Comment 4 Aude 2014-01-07 22:54:42 UTC
https://gerrit.wikimedia.org/r/#/c/101490/ is the patch in question that causes breakage to appear
Comment 5 Aude 2014-01-08 11:29:56 UTC
I looked into how $title is used and it's used for log entries for blacklist hits.

for example:

21:45, 7 January 2014 Aude (talk | contribs | block) caused a spam blacklist hit on Special:NewItem by attempting to add http://j.mp.

In this case, I really think it telling me "Special:NewItem" makes a lot more sense and makes things clear to the user than generating some Q (or P) id that will never become a page since it will be blocked by blacklist.  (and then the id is exhausted / "used" / wasted)  Maybe we could make a fake title like "Item:NewItem" but that's icky.

I think spam blacklist should not use $title in a way that makes assumption the title is associated with the content, at least for new items.  

When the edit is for an existing item or property, then we do have the title and do set it for the context.
Comment 6 Daniel Kinzler 2014-01-08 15:03:09 UTC
Using prepareContentForEdit() just to get a cached version of the parser output seems evil... not to mention that prepareContentForEdit() is pretty bad of and by itself and should be avoided. In any case, it is making assumptions that do not hold here - in this case, the assumption that a page's title is well known before the page has been created.

Also, as Aude said, at least for the log entry it makes much more sense to use the special page's title.

For sharing ParserOutput between, I see two ways:

* use the ParserOutputCache directly
* make getParserOutput cache the last result it returned, and return it again if the parameters are the same.
Comment 7 Brad Jorsch 2014-01-08 15:41:03 UTC
(In reply to comment #6)
> Using prepareContentForEdit() just to get a cached version of the parser
> output
> seems evil... not to mention that prepareContentForEdit() is pretty bad of
> and
> by itself and should be avoided.

Why is that, exactly? See bug 57026, BTW.

It seems to me that calling prepareContentForEdit() in an editing hook makes complete sense.

> * use the ParserOutputCache directly

"ParserOutputCache" doesn't exist.

ParserCache works on the current version of an article, which isn't going to help much when you don't have an article yet and you haven't yet saved the text.

> * make getParserOutput cache the last result it returned, and return it again
> if the parameters are the same.

That doesn't help when not everything uses the same Content object. Or a Content object at all.
Comment 8 Daniel Kinzler 2014-01-08 18:19:01 UTC
(In reply to comment #7)
> It seems to me that calling prepareContentForEdit() in an editing hook makes
> complete sense.

Having an object that represents an edit in progress makes sense, yes. It could have the old Content and Revision, the new Content, and perhaps the new Content with PST applied. But it should be a wrapper object though that generates and remembers parsed versions on demand, not up front. It should also have a well defined interface, not just glued together public members. 

Much could however be already achieved by re-using and passing Content objects.
 
> ParserCache works on the current version of an article, which isn't going to
> help much when you don't have an article yet and you haven't yet saved the
> text.

True. A transient object representing the edit at hand makes more sense.
 
> > * make getParserOutput cache the last result it returned, and return it again
> > if the parameters are the same.
> 
> That doesn't help when not everything uses the same Content object. Or a
> Content object at all.

Everything handling content should use Content objects. Otherwise, how can you handle content without knowing what it is?

And yes, ideally, it would always be the same Content object. There really is no good reason to re-create instances. The Content object should be created when loading from the DB, or when handling a post request - once. 

Trying to avoid re-creation of Content objects would be a good strategy to find and eliminate redundant processing, and would allow for local caching e.g. of the ParserOutput. Perhaps this would be the best approach to address the performance issues described in bug 57026.
Comment 9 Brad Jorsch 2014-01-08 19:59:33 UTC
So until that far-off day when you rewrite all the content handling in all of MediaWiki and all extensions and remove all the old hooks, is Wikidata going to fix this bug so we can re-close bug 57026?
Comment 10 Daniel Kinzler 2014-01-09 13:29:49 UTC
How do you propose Wikidata/Wikibase can fix this? By providing a fake title? There is not "right" title object of an actual page we can provide in this situation. The title isn't know at that point.

The handling in core is largely fixed; legacy hooks are still called, but only if used by an extension that isn't using the respective new hooks yet. 

My proposal is still: cache the ParserOutput in the WikitextContent object, and see if that fixes the redundant parsing issue. And if not, find out why not, and see if we can avoid re-creating Content objects.
Comment 11 Daniel Kinzler 2014-01-09 13:45:59 UTC
Teh quick and dirty fix is to use Q0 as the title, instead of Special:NewItem. That would create a broken/confusing link in the log entry, but should otherwise work "ok".
Comment 12 Brad Jorsch 2014-01-09 15:54:38 UTC
Note that technically there isn't a requirement for a valid title so much as that $context->getWikiPage() returns a WikiPage. You may be able to continue to pass the special page as the title if you manually set an appropriate WikiPage on the context.

BTW, what happens when you try to create a new entity via the API and it hits the spam blacklist?
Comment 13 Daniel Kinzler 2014-01-10 13:19:34 UTC
(In reply to comment #12)
> Note that technically there isn't a requirement for a valid title so much as
> that $context->getWikiPage() returns a WikiPage. You may be able to continue
> to pass the special page as the title if you manually set an appropriate
> WikiPage on the context.

That would be nice and easy if WikiPage was a nicely scoped interface. I don't see a way though to return a WikiPage object for a page that doesn't have a title yet. As it is now, you need a title to construct a WikiPage. We could use the fake Q0 title there, and the special page's title in the context itself... that might work, but it's even more black magick hackery :/
Comment 14 Brad Jorsch 2014-01-10 14:41:49 UTC
(In reply to comment #13)
> That would be nice and easy if WikiPage was a nicely scoped interface. I
> don't
> see a way though to return a WikiPage object for a page that doesn't have a
> title yet. As it is now, you need a title to construct a WikiPage. We could
> use
> the fake Q0 title there, and the special page's title in the context
> itself...
> that might work, but it's even more black magick hackery :/

MediaWiki::initializeArticle(), when given a title that cannot exist, calls Article::newFromTitle()->getPage(), which does new WikiPage(). That apparently works fine with special pages, even though WikiPage::factory() throws errors when given a title that cannot exist.

Gerrit change #106530 just added this same logic to the API's action=parse, for being able to call WikiPage's makeParserOptions() when given a special page title.

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


Navigation
Links