-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC 44: Make "Custom models" the default #44
base: main
Are you sure you want to change the base?
Changes from all commits
441be38
5299449
9df856f
ec46dfa
a52cc07
9e5ce46
9e0b82d
5843785
5655986
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
# RFC 44: Make "custom image model" the default | ||
|
||
* Authors: Karl Hobley | ||
* Created: 2019-10-23 | ||
* Last Modified: 2018-10-23 | ||
|
||
## Abstract | ||
|
||
"Custom image models" is a feature that allows developers to supply a | ||
model for Wagtail to use to store image metadata instead of using the | ||
built-in one. It enables developers to add custom fields to images and | ||
override Wagtail's default image handling behaviour. | ||
|
||
It's challenging to switch to a custom image model once a site | ||
has already made use of Wagtail's built-in image model. Because of | ||
this, we can't add new features that require a custom image model as it | ||
may not be feasible for some users to migrate to one. | ||
|
||
This RFC proposes that we start creating new Wagtail projects with a | ||
custom image model from the start. This change will add a small amount | ||
of extra code to the project template, which I know we would want to | ||
avoid, but I think preventing a potentially arduous migration later | ||
outweighs the overhead of having one extra app in the project. | ||
|
||
This change also applies to image rendition and document models as well. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this would warrant reflecting in the RFC title. |
||
|
||
## Specification | ||
|
||
To implement this RFC, we will make the following changes: | ||
|
||
### Update the project template to use custom image, rendition, and document models | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we also add a custom |
||
|
||
We will add a new app called `assets` into the base project template. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to see alternative proposals for what this app should be called, ideally backed with data on what app name real-world projects put those models in. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we used |
||
This app will contain three models: `Image`, `Rendition` and `Document`. | ||
|
||
All three of these models will exactly match their built-in counterparts | ||
(we will not add an `alt` field to the image model out of the box). | ||
|
||
### Add a custom image model to bakerydemo | ||
|
||
We will add the `assets` app as described in the previous section and | ||
update the fixtures and foreign keys to use this new app. | ||
|
||
### Rename the built-in models to fix naming conflict and discourage their use | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m not clear on whether the effort suggested here is justified? This feels like a lot of potential busywork. |
||
|
||
Currently, custom image models are often called `CustomImage` or | ||
`WebsiteNameImage` because calling the model `Image` would cause the | ||
name of a reverse relation to clash. | ||
|
||
We will not remove the built-in image model, as that would only force | ||
developers to perform the complex migration that this RFC aims to | ||
eliminate for future projects. | ||
|
||
### Tweak the beginners tutorial, so it uses the custom image model | ||
|
||
We will only make necessary tweaks so that it works with the new project | ||
template. I don't think there's any need to describe the image model in | ||
the beginners tutorial. | ||
|
||
### Remove the "Custom image model" advanced topics doc | ||
|
||
We could replace this with a reference document that describes the | ||
methods that developers can override to tweak its behaviour. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here and below – I’m not sure I follow. I’d still call it a custom image model as it’s custom to each project. And the docs would still be useful for the thousands of project that have been existing before the changes suggested here. And the projects that get created without There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My proposal here was to remove the option of not having a custom user model as much as possible, so everyone going forward would create one. This is one of the many things new users need to know about when building their first Wagtail project and I wanted to remove this as an option to reduce decision fatigue. For existing users, we could provide a how-to-guide for "upgrading" to the new favoured approach (while making it clear the old way won't be removed) rather than an "advanced topics" document that implies it's an optional "advanced" feature. I missed out projects that get created without |
||
|
||
### Stop calling it "Custom image model" | ||
|
||
The documentation will be updated to no longer refer to "Custom Image | ||
models" as an extra feature any more. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes!