Add option to prevent migrations from creating initial page/site

[akx]

I feel this is a very elegant solution to the concerns in #1937.

[akx]

Rebased on top of upstream/master.

[akx]

Ah, actually, I found a little bug: the Wagtail migration code forces the root page's numchild to 1, assuming it will immediately get a child (that is has MP_Node properties set manually).

I'll have to fix that...

[gasman]

Hi @akx,
Thanks! This change looks sensible enough to me (subject to the numchild fix being made, of course), but I've never tried loading initial site data through fixtures/migrations, so I'm not really clear about what it achieves in real terms. In other words, I'd like to understand:

• why the initial data is so annoying to deal with (if you're using fixtures, doesn't it get overwritten in the ./manage.py loaddata step anyhow?)
• what a developer would need to change in their process to take advantage of this setting
• how they would benefit as a result

@jordij, @timheap: I'd be interested to hear your thoughts - does this change fit in with the way you're handling initial data?

One thing I'm wary of is adding a settings flag and calling it 'done'... that means we're really just multiplying the number of possible configurations we have to support, without making things better for the average user (who isn't going to read through the reference docs to find out about new features they can use). If this is indeed a better way of doing things - rather than just a different way - then we ought to reflect that in our tutorials and project template where appropriate. Basically, I don't want to fall into the git trap, where there are all kinds of powerful features available, but they're hidden behind obscure switches that only the 'power users' know about because nobody dared to step up with an opinion and say "this is the configuration that 99% of users should be using, so let's set it up that way out of the box" :-)

[mx-moth]

There is a conflict here between new users and power users unfortunately. New users want a functioning Wagtail site out of the box. Providing them with the suggested Site, hope page, and some default user groups makes sense, and makes the initial set up very smooth and nice. Power users might want to create multiple sites, with many different home pages. The initial page structure may not work right for them. They might want a more complicated user and group set up. All this means that the initial sites, pages and groups get in the way, and have to be deleted before the real data gets created.

My preferred set up would be to have Wagtail create no initial data except for the root page of the tree. Not the home page, but the actual root page. No groups, no sites, no pages, nothing. Then, as part of the wagtail start projectname template, a migration should be created that creates these initial pieces of data for the user. New users would have all the initial data required to get started, while power users could customise this initial migration as much as they need to to install their own data - or remove the migration completely and create the initial data in another way.

Sadly, this proposal is quite difficult to achieve in a backwards-compatible manner, without having a flag new projects can set that indicates this initial data will be created by the project, which is partially what this PR is about.

[jordij]

I fully agree with Tim on this one

[timorieber]

Agreed. Nothing more to say to Tim's explanation.

[gasman]

So, if I've understood correctly - this PR could be turned into @timheap's ideal setup (or at least an approximation to it...) by:

• leaving the WAGTAIL_MIGRATE_INITIAL_* settings as True by default (to preserve backwards compatibility on existing projects);
• but setting them to False in project_template's settings (and presumably, in our 'integrating with Django' / 'configuring Django for Wagtail' docs, advising developers to do the same);
• and updating project_template's data migration (currently home/migrations/0002_create_homepage.py) to create the initial site / homepage / group records.

The settings would have to remain in the project template for the foreseeable future - so effectively we're in the same situation as Django's USE_I18N / USE_L10N / USE_TZ flags, which have been in everyone's settings files forever and rarely need to be changed, but can't be dropped due to ancient legacy reasons. Which would be unfortunate, but clearly the Django world has tolerated those redundant settings without too much heartache, so it's not the end of the world if we have to do the same here ;-)

However, it seems to me that all we've really gained in all of this is the opportunity to omit a few 'delete' lines from the 0002_create_homepage migration - in other words, it boils down to a choice between some extra noise in the project_template settings, or some extra noise in the project_template migration. And given that choice, I'd go for the noise in the migration - it's something that only power users need to look at, and if it was suitably well-commented, perhaps with sample commented-out code blocks that users could uncomment and adjust to fit their requirements ("to remove the default groups, do this; to load initial data from a fixture, do this...") then I could see that being a lot neater. (Admittedly, we'd have to give a similar treatment to the configuration / integration docs.)

As I said, I'm not fully clear on the motivation for this change, so I'm prepared to accept that there may be issues with the "big noisy migration" approach that I haven't grasped.

[akx]

Rebased again, sorry for the delay.

As for my motivation -- well, @timheap kinda put it in words for me there :)

I find it silly that the default project template's migration actually deletes the objects another migration has created, just so it can create its own versions, with the correct content types etc.

We're evaluating Wagtail for a home page project here at @andersinno (and so far it's been pretty great, though I'm looking forward for proper i18n support, as projects here in Finland tend to be multilingual beasts -- but I digress), and for that project we've been running a fork release that has this patch in, and our initial migration (though it could just as well be a "seed initial data" management command) looks like this. No silly deleting or editing of existing useless data. :)

def create_homepage(apps, schema_editor):
    ContentType = apps.get_model('contenttypes.ContentType')
    Site = apps.get_model('wagtailcore.Site')
    HomePage = apps.get_model('superawesomeprojectyay.HomePage')
    homepage_content_type = ContentType.objects.get_for_model(HomePage)
    homepage = HomePage(title='Kotisivu', slug='home', content_type=homepage_content_type)
    root = Page.objects.get(slug='root')
    root.add_child(instance=homepage)
    Site.objects.create(hostname='localhost', root_page=homepage, is_default_site=True)
    ContentType.objects._cache.clear()  # See https://github.com/django/django/pull/6088

[gasman]

I agree that the current behaviour is silly - no argument there :-)

(Sorry for the wall of text that follows, but it's kind of important...)

If we're making this change, it's absolutely essential that we roll out the new setting in a way that encourages all new users to use it - i.e. we enable it in the project template by default. For me, the worst possible outcome is that we end up with two competing conventions in use - the current "silly" behaviour, and the fixed behaviour that only power users know about - because then we end up with twice as many scenarios to test against, harder-to-diagnose tech support problems ("why are they getting that weird behaviour? Oh, they must have Obscure Setting X enabled..."), more complications if we change it again in future (see #1824 where we've tied ourselves in knots because a 'clever' migration had unforeseen side effects, and now we have two diverging configurations to deal with) - and just generally, more "moving parts" to account for. I fear that's what will happen if we merge this PR in its current form.

(Besides making life more difficult for us Wagtail developers, the "opt-in setting" approach doesn't really improve the developer experience for end users either. 95% of users won't read the reference docs, so they won't learn about the setting at all, and will be stuck with the "silly" behaviour. The other 5% won't understand the purpose of the setting, unless they've learnt a LOT about Wagtail internals, such as the difference between wagtailcore migrations and project_template migrations, and why having a raw Page instance for your homepage is a bad thing. Nobody is going to learn all those details for their first Wagtail project, so the only people who will benefit from this setting are people like you, who have been previously bitten by the "silly" behaviour, realised it's silly, and started digging into Wagtail internals for a better solution. That's not a good experience for anyone.)

So, unless anyone can see an alternative approach that addresses my concerns above, I propose that we add explicit WAGTAIL_MIGRATE_INITIAL_PAGE_AND_SITE = False / WAGTAIL_MIGRATE_INITIAL_GROUPS = False lines into project_template/project_name/settings/base.py. The question remains: if we do that, is this change still worthwhile, or does that completely nullify the goal of finding a more elegant solution? Personally, it seems like the latter to me: we no longer have the silly 'delete' operations sitting in migration 0002 forever, but instead we have some silly "don't change this unless you know what you're doing" lines sitting in the settings file forever. But then it's not really my judgment call to make - clearly you (@akx, @timheap, @jordi, @timorieber) have stronger feelings about the usefulness of this change than I do - if you think this PR is still worthwhile with my proposed amendment, I'll happily go along with it.

[gasman]

I'll take the silence as a sign that there's no enthusiasm for my amended proposal, then :-)

Unfortunately I don't think we're going to find a solution to the 'ugly migrations' issue that's satisfactory to everyone here and doesn't just shift the ugliness somewhere else. As such, I'll close this issue (but as always, it can be reopened if there's further progress on this).

[jsan3386]

Maybe because the emails are mixed up again? I have no idea why i keep
receiving these kind of messages. I don't know why github accounts are
always mixed up. Have nothing to do with that project.
On Apr 8, 2016 9:18 PM, "Matt Westcott" notifications@github.com wrote:

Closed #2112 #2112.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2112 (comment)

[gasman]

@jordi Oops, that was my fault... looks like I mentioned you instead of @jordij in my earlier message. Sorry for the spam!

(Github doesn't provide a way for me to unsubscribe you from notifications, as far as I can see - you can unsubscribe yourself at #2112)

[jordij]

Oops sorry @gasman I didn't get notified about what was going on.

I agree with you on the_ " Nobody is going to learn all those details for their first Wagtail project, so the only people who will benefit from this setting are people like you, who have been previously bitten by the "silly" behaviour..."_ but to me this change is useful as I'd use the new behaviour in 99% of my Wagtail-based implementations.

Personally, I don't see it as an issue if it remains as a "hidden" setting that shouldn't be changed unless the developer really wants to. Django has several settings with a default value that remain kind of "hidden" until you start digging and learning about them. I feel this would be a similar case, but again this is just my opinion :)

[dopry]

As is, this PR looks fine. Backward compatible behavior is preserved by default. If a users wants to change the setting they can, and the caveat that it won't work with the default project seems to be clear in the docs.

@gasman

If we're making this change, it's absolutely essential that we roll out the new setting in a way that encourages all new users to use it - i.e. we enable it in the project template by default. For me, the worst possible outcome is that we end up with two competing conventions in use - the current "silly" behaviour, and the fixed behaviour that only power users know about - because then we end up with twice as many scenarios to test against, harder-to-diagnose tech support problems ("why are they getting that weird behaviour? Oh, they must have Obscure Setting X enabled..."), more complications if we change it again in future (see #1824 where we've tied ourselves in knots because a 'clever' migration had unforeseen side effects, and now we have two diverging configurations to deal with) - and just generally, more "moving parts" to account for. I fear that's what will happen if we merge this PR in its current form.

1. It is not necessary to force all users to use this approach. The default behavior should preserve backward compatibility so typical users are unaffected. Core doesn't want or need to support this option, and can clearly state as much in the documentation. If the community finds this functionality useful, it may make sense for core to revisit the question of support.

(Besides making life more difficult for us Wagtail developers, the "opt-in setting" approach doesn't really improve the developer experience for end users either. 95% of users won't read the reference docs, so they won't learn about the setting at all, and will be stuck with the "silly" behaviour. The other 5% won't understand the purpose of the setting, unless they've learnt a LOT about Wagtail internals, such as the difference between wagtailcore migrations and project_template migrations, and why having a raw Page instance for your homepage is a bad thing. Nobody is going to learn all those details for their first Wagtail project, so the only people who will benefit from this setting are people like you, who have been previously bitten by the "silly" behaviour, realised it's silly, and started digging into Wagtail internals for a better solution. That's not a good experience for anyone.)

2. I can say 100% an opt-in setting would improve my developer experience with wagtail. I've lost a half-day, just figuring out how to set up fixtures so that I can prep to kick off the front-end development on my project. I've had to figure out our normal approach with fixtures doesn't work because someone chose to use a data migration to seed data in wagtail, rather than fixtures. Then I had to work around the fact the manage.py flush doesn't work and build that into our docs and docker-compose files. You acknowledge the problem and silliness of having the seed data embedded in a data migration to begin with, such as slowing down test setup and breaking the fixtures feature.

So, unless anyone can see an alternative approach that addresses my concerns above, I propose that we add explicit WAGTAIL_MIGRATE_INITIAL_PAGE_AND_SITE = False / WAGTAIL_MIGRATE_INITIAL_GROUPS = False lines into project_template/project_name/settings/base.py. The question remains: if we do that, is this change still worthwhile, or does that completely nullify the goal of finding a more elegant solution? Personally, it seems like the latter to me: we no longer have the silly 'delete' operations sitting in migration 0002 forever, but instead we have some silly "don't change this unless you know what you're doing" lines sitting in the settings file forever. But then it's not really my judgment call to make - clearly you (@akx, @timheap, @jordi, @timorieber) have stronger feelings about the usefulness of this change than I do - if you think this PR is still worthwhile with my proposed amendment, I'll happily go along with it.

I would reject your proposal based on your false premise that this change has to break backward compatibility. I think it is perfectly okay to give power users some freedom. Nothing changes in settings unless a user turns it on. Core can take a wait an see approach and see if the support concerns are valid, or maybe the contributor community will develop a new practice core can adopt. I'd say all the PR needs is clear documentation that core won't support this.