Skip to content

Conversation

nekiro
Copy link
Member

@nekiro nekiro commented Nov 23, 2021

Honestly, idk why it's NOT NULL as this is binary, but it only triggers me when I manually add entries thru phpmyadmin and it spits out errors, cause conditions cannot be null.

Default value for this should be null, this should not affect anything in loading and will fix that annoyance.

@@ -37,7 +37,7 @@ CREATE TABLE IF NOT EXISTS `players` (
`posx` int NOT NULL DEFAULT '0',
`posy` int NOT NULL DEFAULT '0',
`posz` int NOT NULL DEFAULT '0',
`conditions` blob NOT NULL,
`conditions` blob DEFAULT NULL,
Copy link
Contributor

@marmichalski marmichalski Nov 23, 2021

Choose a reason for hiding this comment

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

This seems like a sane change, but considering there are already projects using that field, for example web AACs as string, it might be reasonable to do DEFAULT ''?

Nevermind. MySQL does not support default values for blob 🙄.

@ghost ghost merged commit f2c81c8 into master Nov 26, 2021
@ghost ghost deleted the nekiro-patch-2 branch November 26, 2021 03:59
@DSpeichert
Copy link
Member

Hold up! Where the migration is?

@nekiro
Copy link
Member Author

nekiro commented Nov 26, 2021

Hold up! Where the migration is?

It does not break/change anything, so there is none.
I don't think we need migration for that xD

@Zbizu
Copy link
Contributor

Zbizu commented Nov 26, 2021

would conflict with upcoming version 12 migrations

@DSpeichert
Copy link
Member

Hold up! Where the migration is?

It does not break/change anything, so there is none. I don't think we need migration for that xD

It does change schema, so now you introduce divergence... wouldn't hurt to have a migration for that.

Znote pushed a commit to Znote/forgottenserver that referenced this pull request Jan 30, 2022
DSpeichert pushed a commit that referenced this pull request May 9, 2022
This pull request was closed.
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.

4 participants