Skip to content

Conversation

@jbowen116
Copy link

Modify 'protected function ensureEntityColumns()' to set a default of ' '. Fixes the broken automatic update from 17.0.10 to 18.0.10.

Specifically implements the solution commented here: #23174 (comment)

Modify 'protected function ensureEntityColumns()' to set a default of ' '. Fixes the broken automatic update from 17.0.10 to 18.0.10. 

Specifically implements the solution commented here: nextcloud#23174 (comment)
@ChristophWurst
Copy link
Member

This doesn't seem right. If the column can contain empty strings, then it should be nullable. Oracle would choke on that as well, right @nickvergessen?

@blizzz
Copy link
Member

blizzz commented Nov 19, 2020

This doesn't seem right. If the column can contain empty strings, then it should be nullable. Oracle would choke on that as well, right @nickvergessen?

This is also what I understand.

default should be removed then, I suppose?

@juliusknorr
Copy link
Member

Yes, see 32d7459 for how to properly migrate a column from notnull => true, to notnull => false.

You will need to adjust the existing migration so that it is properly set on new Nextcloud installations as well as adding a new migration step to fix the column on existing installations.

@jbowen116
Copy link
Author

This doesn't seem right. If the column can contain empty strings, then it should be nullable. Oracle would choke on that as well, right @nickvergessen?

I agree with you. Before I considered looking at this code, I manually created that column and made it nullable. That didn't work for me, either. I assumed the code in the update was still choking on the changes (maybe trying to 're-do' the database changes?). It's almost certain there's a better fix than what I've proposed here...I don't love the idea of creating single-space DEFAULT entries, either.

'notnull' => true,
'length' => 256,
'default' => '',
'default' => ' ',
Copy link
Member

Choose a reason for hiding this comment

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

Would not removing this be the proper way to go?

Copy link
Member

Choose a reason for hiding this comment

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

@blizzz Might know exactly but iirc the entity might be empty for old workflowengine rules from before Nextcloud 18 when migrating.

Copy link
Member

Choose a reason for hiding this comment

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

In 18 we have a repair steps that fills the column. This is done after migration, because the column in question did not exist in 17. Setting the default to \OCA\WorkflowEngine\Entity\File::class would then be the right thing, as this is what the post migration job would do to any column that appears empty. Which would render this part of the post-migration script unnecessary, if I do not oversee anything.

Copy link
Member

Choose a reason for hiding this comment

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

@blizzz Mind to do this change in a PR directly?

@blizzz
Copy link
Member

blizzz commented Nov 23, 2020

replaced by #24315 for #24208 (comment)

@blizzz blizzz closed this Nov 23, 2020
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