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

Manually set Types #801

Merged
merged 2 commits into from Feb 26, 2021
Merged

Manually set Types #801

merged 2 commits into from Feb 26, 2021

Conversation

jotoeri
Copy link
Member

@jotoeri jotoeri commented Feb 23, 2021

I'm not really a fan of this, but seems like we need to do this to fix the support for NC19.

Fixes #799

@jotoeri jotoeri added bug Something isn't working 3. to review Waiting for reviews labels Feb 23, 2021
@jotoeri jotoeri added this to the 2.2 milestone Feb 23, 2021
@skjnldsv
Copy link
Member

I'm not really a fan of this, but seems like we need to do this to fix the support for NC19.

Aaaah, damn!

@nickvergessen
Copy link
Member

https://github.com/nextcloud/3rdparty/tree/stable20/doctrine/dbal/lib/Doctrine/DBAL/Types works with 20+
but inlining is also fine until you drop 19

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Please open a ticket for the cleanup once anything older than 20 was dropped so you can use the new Nextcloud constants.

@@ -55,6 +54,11 @@ class Version010200Date20200323141300 extends SimpleMigrationStep {
'dropdown' => 'multiple_unique'
];

private const TYPE_BOOLEAN = 'boolean';
private const TYPE_INTEGER = 'integer';
private const TYPE_JSON = 'json';
Copy link
Member

Choose a reason for hiding this comment

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

:-X does this even work on all dbs? at least not in our mappers?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to doctrine-docs, doctrine will automatically fall back to text in case a DB does not support json. It worked flawless until now...
I just tested this branch with clean installations of all three dabase-types and it works without problems...

Copy link
Member

Choose a reason for hiding this comment

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

@nickvergessen you're worrying about oracle?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah

Copy link
Member

Choose a reason for hiding this comment

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

Can we easily add testing ? Do you have a github workflow file on hand we could steal?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

@jotoeri can you add this test to this pr? :)

Copy link
Member

Choose a reason for hiding this comment

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

Then, once green, you can merge! 🚀

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 26, 2021
Fixes NC19 Support

Signed-off-by: Jonas Rittershofer <jotoeri@users.noreply.github.com>
Signed-off-by: Jonas Rittershofer <jotoeri@users.noreply.github.com>
@jotoeri jotoeri merged commit 6360a64 into master Feb 26, 2021
@jotoeri jotoeri deleted the fix/nc19 branch February 26, 2021 19:40
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Mar 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Migration not working on NC18 & NC19!
4 participants