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

[3.0] Fix exdates not having a default value #8235

Open
wants to merge 2 commits into
base: release-3.0
Choose a base branch
from

Conversation

frandominguezl
Copy link
Member

I was getting an error upon installing that this field didn't have a default value.

@DiegoAndresCortes
Copy link
Member

Spotted this with @Sesquipedalian
But BLOB and TEXT columns cannot have DEFAULT values. is still in the documentation.

Something must've changed in 8.0 because SMF 2.1 was already using STRICT_TRANS_TABLES

@frandominguezl
Copy link
Member Author

Indeed, the MySQL 8.0.13 release notes include this feature. I added the default value to Postgres as well, but I don't know if that's needed.

@Sesquipedalian
Copy link
Member

Sesquipedalian commented May 24, 2024

The real question is, why is the installer complaining about this for the exdates column but not for the rdates column that is identical in every way except for the name itself? Even if recent versions of MySQL have added support for default values for text columns, default values are not normally required for them. So we must be doing something to trigger this error message. We need to figure out what are we doing and how to fix it properly.

@frandominguezl
Copy link
Member Author

Understood, I will try to dig into this this weekend

@Sesquipedalian
Copy link
Member

Cool. Thanks!

@Oldiesmann
Copy link
Contributor

Sounds like we might be trying to insert data into that particular table without setting a value for that column. That's usually what triggers the "column doesn't have a default value" error.

@frandominguezl
Copy link
Member Author

Oldiesmann is actually right:
https://github.com/SimpleMachines/SMF/blob/release-3.0/other/install_3-0_MySQL.sql#L1576

My preference for this kind of things is to actually set the default value upon table creation, but as I understand it, it won't be compatible for MySQL versions prior to 8.0.13 so, might just tweak that query to include the default value for exdates.

Am I right? :)

@Oldiesmann
Copy link
Contributor

Either option will work actually. The current minimum MySQL version for 3.0 is 8.0.35, and it would appear that Postgres also supports default values for text columns. Since it's just one query though it might be easier to just fix that query.

@sbulen
Copy link
Contributor

sbulen commented Jun 13, 2024

The real problem here is we are now inconsistent.

IMO, we should be adding defaults on ALL the text values. This requires updates throughout the installer & upgrader.

@albertlast
Copy link
Collaborator

We didn't done this,
because of the mysql version we supported in 2.1.
But yea 3.0 got new rules.

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

Successfully merging this pull request may close these issues.

6 participants