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

Remove unecessary constraint on database name #19654

Closed
wants to merge 1 commit into from
Closed

Conversation

vaab
Copy link

@vaab vaab commented Feb 26, 2020

Installation of nextcloud prevents database name to have dots inside.

This is not a limitation of database (Postgres and Mysql supports this), but an added constraint added out of the blue.

Nextcloud should not add limitation of their own. I'm installing broad list of services, and none of them have ever decided to enforce their own naming constraints by preventing their installation if we don't follow them. I'm using in my case the domain name of the service as database name (as another reporter of this bug did).

This topic was mentioned a few time in issues in owncloud, but these are locked and closed. However, it hits nexcloud the same.

owncloud/core#19390
owncloud/core#20381

Removing these 3 lines will make everything work for me on postgres...

How can I help more ? Many thanks.

@kesselb kesselb added 3. to review Waiting for reviews enhancement labels Feb 26, 2020
@kesselb
Copy link
Contributor

kesselb commented Feb 26, 2020

@nickvergessen
Copy link
Member

if (substr_count($string, '.')) {
list($alias, $columnName) = explode('.', $string, 2);

and other places will break hard. The check is there for a reason.

Simply deploy your databases as s/./_ and be done without problems.

@kesselb
Copy link
Contributor

kesselb commented Feb 26, 2020

https://github.com/owncloud/core/pull/37021
https://github.com/owncloud/core/pull/37020

Seems to be okay for ownCloud. The above code looks like something to split column names. Also the test suite seems to be happy (apart the usual hicups) 😕

I agree with you and also the stackoverflow thread that using dots in database names is just calling for trouble. But if people want to run such a setup ... fine by me ;)

@vaab
Copy link
Author

vaab commented Feb 27, 2020

If there were any problems on nextcloud's side to manage dotted database name, this is a nextcloud issue, and a concerning one, definitively not on the admin/user side.

MySQL and postgres move toward supporting dotted name was a reflected move, and is supported in all the other services I have deployed until now. nextcloud seems to support it also, if you remove this ad-hoc test.

Besides, I don't feel that it is of your responsibility to "protect" users/dev/devops by not allowing to use these features.

@vaab
Copy link
Author

vaab commented Mar 25, 2020

FYI, this was reviewed, validated, tested and merged on owncloud's side.

owncloud/core#37020

@gingir
Copy link

gingir commented Aug 3, 2020

using dots in database names is just calling for trouble. But if people want to run such a setup ... fine by me ;)

never had this issue with all the other php apps (wordpress, joomla, drupal, phpbb, moodle to name but a few)

there is nothing wrong with using dots in database names, that's why MySQL, MariaDB, PostgreSQL support it.

and there is something called "escaping special characters", you know.

so, vaab is 100% spot on.

besides, the error "please match the requested format" is not helpful at all.
nextcloud looks like a great app i am eager to try, it should just follow suit.

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

Successfully merging this pull request may close these issues.

4 participants