search

octobercms / install

Installation wizard for October CMS
122 stars 91 forks source link

Add prefix field to database config page #82

Closed dshoreman closed 8 years ago

dshoreman commented 8 years ago

All the handling was in place for a table prefix, but there was no input for it in the form. This commit adds the field to each of the provider forms -- and adds the variable to the php side for sqlite as well while we're at it.

daftspunk commented 8 years ago

We don't officially support table prefixes.

dshoreman commented 8 years ago

@daftspunk Any particular reason why not? It seems to (and should) work for the most part, since the majority of it is handled at the core in Eloquent etc.

I didn't look into prefixes with October at all until someone had a problem with Octoshop - that's when I Googled and found a single prefix-related bug report for October. With that one solved I doubt there's much else wrong with prefixes, and it had nothing to do with the Octoshop issue either (that one was totally my fault).

Granted, I don't use prefixes, nor do I know many that do. However October's target audience - most being on shared hosts - are more likely to need a prefix due to database quotas. It seems strange to deny those people such a basic config option just because they have to use the GUI installer.

daftspunk commented 8 years ago

The ratio of support to usage is too high. It works, sure, and the PR fixes are still accepted. We may support it officially in the future.

On Tuesday, 26 January 2016, Dave Shoreman notifications@github.com wrote:

@daftspunk https://github.com/daftspunk Any particular reason why not? It seems to (and should) work for the most part, since the majority of it is handled at the core in Eloquent etc.

I didn't look into prefixes with October at all until someone had a problem with Octoshop - that's when I Googled and found a single prefix-related bug report for October. With that one solved I doubt there's much else wrong with prefixes, and it had nothing to do with the Octoshop issue either (that one was totally my fault).

Granted, I don't use prefixes, nor do I know many that do. However October's target audience - most being on shared hosts - are more likely to need a prefix due to database quotas. It seems strange to deny those people such a basic config option just because they have to use the GUI installer.

— Reply to this email directly or view it on GitHub https://github.com/octobercms/install/pull/82#issuecomment-174809824.

lbialy commented 8 years ago

We vote strongly nay. This is a craptastrophe waiting to happen, since official convention for table names is already vendor_plugin_model and if you think about it, for pivot tables its vendor_plugin_model1_model2. Right now plugin vendor can test his migrations during development and it's rather improbable that migration will fail at customer / user installation if it had not blown up in development (because too long table name, we've already seen it with pivot tables). This should not only be discarded, there should be a big sign "WARNING! DRAGONS AHEAD!" in docs and in comment inline with this setting telling people that using table prefixes can lead to serious problems.

lbialy commented 8 years ago

Also, one db per installation. Why use prefixes at all?

dshoreman commented 8 years ago

@daftspunk Would you be opposed to adding the field, but having it hidden somehow by default? Methods that spring to mind could involve a hidden/hard to find 'Advanced' checkbox, or requiring the user input the Konami Code before showing the dbPrefix input. Also, since you mention acceptance of PR fixes, I'll just drop a shameless plug right here :)

@lbialy There's not a lot we can do about table name lengths. The only time that's an issue is when using foreign keys (I've encountered it beforewith my plugins) so the workaround there is to simply not use foreign keys. Downside is it's going to sacrifice some speed for larger applications, but that's another subject entirely.

Ref. one db per installation, I totally agree. I also think all those people unlucky enough to be on bad shared hosting plans with single-db restrictions and outdated PHP versions should either complain to their host, find another host, or both. Reality is people can't or won't always move to a better host, they'll just use Wordpress or something else that does what they think they need.

lbialy commented 8 years ago

Not using foreign keys (in internal plugin context, inter-plugin FKs get tricky because of plugin:refresh, but can be done) means you risk data corruption and orphan rows, because transactions support in october core and most plugins is non-existent (therefore db crash or network split or whatever between ie issuing DELETE FROM tbl WHERE id ? and afterDelete handler firing and issuing DELETE FROM children_tbl WHERE parent_id ? will result in orphans). And I am disregarding performance here completely.

My opinion stands unchanged. /Lucas