Page MenuHomePhabricator

B8. Cleanup import code further following occupation/ensure changes
Closed, ResolvedPublic2 Story Points

Description

See 720d146e52b227434b656e5cea99f66184312b40 and d2f37d2cfeca75fc25ac9687d5b3e93bc8ecf300 . After that, the import code in OccupationListener no longer worked (since it relies on a different workflow/page insertion order), so I made fixes in the Import directory.

However, OccupationListener now needs to be cleaned up (possibly eliminated).

Event Timeline

Mattflaschen-WMF claimed this task.
Mattflaschen-WMF raised the priority of this task from to Needs Triage.
Mattflaschen-WMF updated the task description. (Show Details)
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
DannyH edited a custom field.May 6 2015, 7:01 PM
DannyH renamed this task from Cleanup import code further following occupation/ensure changes to B8. Cleanup import code further following occupation/ensure changes.May 6 2015, 8:45 PM
Catrope removed Mattflaschen-WMF as the assignee of this task.Jun 2 2015, 5:39 PM

Looking at TalkpageImportOperation::import (Importer.php), I don't really see anything that needs to be cleaned up. Everything's geared up the way it's supposed to.
It's slightly verbose, but when fixing the pageId stuff, I made the deliberate choice of having to explicitly allow creation & ensure revision.
I'm not sure we should dumb down how this is implemented (feel free to disagree!) - we previously relied on a bit of magic for these things, but that didn't work correctly.

OccupationListener shouldn't be eliminated yet. It's irrelevant for "occupation" (so should probably be renamed) since it doesn't create the board page (workflow_page_id).
What it does is creating a page for every Topic:xyz (although I'm not sure why exactly we need that - I would assume to be more in line with MW core, so Title::exists etc work?)

Mattflaschen-WMF added a comment.EditedJun 5 2015, 4:15 AM

OccupationListener shouldn't be eliminated yet. It's irrelevant for "occupation" (so should probably be renamed) since it doesn't create the board page (workflow_page_id).

The bug is about the if block at https://git.wikimedia.org/blob/mediawiki%2Fextensions%2FFlow.git/ae7f9b97e956a005dfc93772be4d8f62a73ea318/includes%2FData%2FListener%2FOccupationListener.php#L61 apparently doing nothing useful for 'imported' content. It looks like it *should* do what TalkpageImportOperation::import is doing, but it didn't work anymore (hence me adding the TalkpageImportOperation::import code) due to refactoring.

What it does is creating a page for every Topic:xyz (although I'm not sure why exactly we need that - I would assume to be more in line with MW core, so Title::exists etc work?)

Yeah, we definitely need topic pages to exist for every topic as part of moving towards standard content model and away from weirdness. When this is wrong (which it shouldn't be anymore) there were e.g. sometimes red links for topics. Not proposing any change to that part.

Change 216092 had a related patch set uploaded (by Matthias Mullie):
No need to allowCreation on NS_TOPIC, it's always occupied

https://gerrit.wikimedia.org/r/216092

Change 216092 merged by jenkins-bot:
No need to allowCreation on NS_TOPIC, it's always occupied

https://gerrit.wikimedia.org/r/216092

Is this task done now that https://gerrit.wikimedia.org/r/216092 is merged?

Yes, this is the code area I was thinking of.

DannyH closed this task as Resolved.Jun 15 2015, 11:35 PM
DannyH added a subscriber: DannyH.