Skip to content
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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Conversation

kaedroho
Copy link
Contributor

@kaedroho kaedroho commented Oct 23, 2019

@kaedroho kaedroho added the 1:New label Oct 23, 2019
@kaedroho kaedroho changed the title Make "Custom image model" the default RFC 44: Make "Custom image model" the default Oct 23, 2019
text/044-image-models.md Outdated Show resolved Hide resolved
text/044-image-models.md Outdated Show resolved Hide resolved
text/044-image-models.md Outdated Show resolved Hide resolved
text/044-image-models.md Outdated Show resolved Hide resolved
text/044-image-models.md Outdated Show resolved Hide resolved

### 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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something similar would need to be done for the Custom document model documentation page.

@lb-
Copy link
Member

lb- commented Oct 27, 2019

Hey, finally got the chance to read this and 100% agree that this would be helpful and enable a lot of future improvements over time.

I have also put up an issue on the main repo to add some fields to the image/document models that would be really useful wagtail/wagtail#5658.

Finally, I did some analysis of about 78 different document/image models I found on Github projects. This will give a bit of an indicator of the kinds of changes made by developers.

https://docs.google.com/spreadsheets/d/1JgrDb5s_ugAnL04r3GOBqh5DodZodqqp3rqrN1sTKpE/edit?usp=sharing

The top ten fields added in custom models are:

  • 16 x caption probably because it is in the docs
  • 12 x credit
  • 11 x alt / alt_text yes, I know we are not adding this and I agree but just FYI
  • 9 x description / details / summary
  • 7 x author
  • 7 x source / source_name
  • 5 x source_url / external_url
  • 4 x attribution
  • 4 x file
  • 4 x licence / license
  • 3 x copyright_year / year

@kaedroho kaedroho added 2:Accepted and removed 1:New labels Oct 29, 2019
@kaedroho
Copy link
Contributor Author

We discussed this as a croe team meeting so I think this is at least stage 2

Copy link
Member

@thibaudcolas thibaudcolas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ref wagtail/wagtail#5789 – I’d love to see this happen. There are a few things here I’d like to see changed but they feel pretty minor. In particular more consideration on the app name, whether a rename of the existing models is worthwhile, and consideration to include a custom User model as well.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would warrant reflecting in the RFC title.


### Update the project template to use custom image, rendition, and document models

We will add a new app called `assets` into the base project template.
Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we used media (and added custom wagtailmedia classes there as well)

### 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.
Copy link
Member

Choose a reason for hiding this comment

The 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 wagtail start?

Copy link
Contributor Author

@kaedroho kaedroho May 24, 2024

Choose a reason for hiding this comment

The 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 wagtail start in this doc, perhaps we should add these models to the "adding to an existing django project" doc as well.

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
Copy link
Member

@thibaudcolas thibaudcolas Oct 6, 2023

Choose a reason for hiding this comment

The 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.


To implement this RFC, we will make the following changes:

### Update the project template to use custom image, rendition, and document models
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we also add a custom User model, based on the same rationale?

@thibaudcolas thibaudcolas self-assigned this Oct 6, 2023
@thibaudcolas thibaudcolas changed the title RFC 44: Make "Custom image model" the default RFC 44: Make "Custom models" the default Oct 20, 2023
@rgs258
Copy link

rgs258 commented Apr 18, 2024

Any chance of a custom Page model :)

@thibaudcolas
Copy link
Member

@rgs258 😄 can you say more?

@NotANormalNerd
Copy link

@thibaudcolas I can elavorate. It would be amazing to have a Swappable Page model, like the user model, so we can add custom fields and other stuff to the base page. Right now we have to fork wagtail and add changes to the base page there.
We can't add a "additonal" model via foreign key for performance and other reasons.

So to have the a custom page model by default, that can inherit from a "AbstractBasePage" would be amazing. Wagtail could work with the AbstractBasePage and people could add whatever they need to their Page Model.

@Stormheg
Copy link
Member

We briefly discussed this RFC again during the core team meeting today. This RFC needs answers to the remaining comments by Thibaud and we can move this to final comment period after. Keen to see this become the default 👍

@kaedroho would you be okay with me taking over this RFC? Or would you rather implement it yourself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants