search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[DX] Switch to a simplified array syntax for database connection information in settings.php #2231

Open olafgrabienski opened 8 years ago

olafgrabienski commented 8 years ago

I just tried to install Backdrop 1.5 in a local AMPPS environment. During installation, after being on the language page, I got the message

Fatal error: Class 'DatabaseTasks_' not found in {...}/core/includes/install.inc on line 1362

and wasn't able to continue the installation.

Line 1362 concerns the database driver. Googling something like drupal error database driver led me to a comment on d.org regarding Backup and Migrate about a problem with special characters in the database password string. I had a look in my settings.php, noticed a forward slash in the password string, changed the database password in the relevant places and was able to continue the installation.

Are forward slashes (and possibly particular other special characters) in the database password string not supported by Backdrop for a certain technical reason? If so, we should document it somewhere. If not, can we fix that?

ADVOCATE: @yorkshire-pudding

Dinornis commented 7 years ago

I experienced a similar issue with a password that included special characters, the only difference was that I did not see an error message, just a blank page during install. Not sure what would be the best way to address this.

olafgrabienski commented 7 years ago

Today I got the same, or very similar, issue again. Installation of Backdrop failed, displaying the general message "The website encountered an unexpected error. Please try again later."

When I went back to the site URL without the installer related query strings, I got a more specific notice:

Notice: Undefined index: driver in {...}/core/includes/database/database.inc on line 1718

As in the first post above, it looks again like a database driver related issue. (Maybe sort of misleading, because the driver is the first element of $database in settings.php.) I was however able to install Backdrop with a new database using a new password, and I guess that worked because there were no 'problematic' characters in the string.

So I ask again: are particular special characters not supported in the database password string?

LeeteqXV commented 7 years ago

I just ran into this on a test installation of 1.6.3, where the password contained a semicolon (;) and a hash (#) character on two different/adjacent locations in the password. I replaced those two with underscores (so that at least it had 1 special character in it)(using the cPanel tool to update/change existing dbuser passwords), reloaded the error page, and the installation continued without any further problems.

olafgrabienski commented 7 years ago

Documenting for testing purposes: characters mentioned in the posts above that might be problematic:

Feel free to add more.

daggerhart commented 7 years ago

This is because the bootstrap is using parse_url() to load this string into an array, so anything that is part of the url schema is problematic.

Edit: Found documentation related to using the array format in settings.php. Tested and it works. This should allow special characters without issue.

complex-password

Edit 2: Deleting all my custom code non-sense. I think we should instruct people to use the array syntax in settings.php.

olafgrabienski commented 7 years ago

@daggerhart Do you suggest to change settings.php, so that the default would be to use the array format instead of the simple database configuration?

laryn commented 7 years ago

I just hit this using an autogenerated password. It was a big pain to sort out as things had already been partially installed and I had to:

daggerhart commented 7 years ago

@olafgrabienski sorry, i missed your message to me.

Yes, if we change to using the array format during settings.php generation, we'll avoid users running into issues like @laryn just experienced.

I don't know of any great reason to using the single string/url format. As a user, I think it is much harder to understand because you have to be somewhat familiar with authentication based url syntax and avoid its special characters.

Whereas the array syntax is very clear about what each value is, and (as far as i know) doesn't have any limitations concerning special characters.

olafgrabienski commented 7 years ago

I don't know of any great reason to using the single string/url format [for database configuration in settings.php].

@quicksketch @jenlampton To fix errors during installation due to special characters in the database password: Do you think it's possible to change the database configuration in settings.php, so that it uses the array format instead of the single string format? Or is there a particular reason why Backdrop has to use the single string format?

jenlampton commented 7 years ago

Or is there a particular reason why Backdrop has to use the single string format?

@olafgrabienski backdrop does not need to use the single string format, the array format from Drupal 7 will work as well.

olafgrabienski commented 7 years ago

Thanks @jenlampton ! To change settings.php in such an essential part seems a big change to me. Does it make sense, and are there caveats? I'd appreciate some more feedback from experienced Backdrop developers before we make a PR!

jenlampton commented 7 years ago

There's no need for a PR, backdrop already works this way.

On your site: copy the array syntax from a D7 settings.php file and see if that fixes the problem. If so we can close this issue.

laryn commented 7 years ago

@jenlampton If we want it to use the array syntax by default on new installs (to avoid the special characters in the MySQL password causing a problem) we'd at least need a PR to change the default settings.php file, wouldn't we? And maybe some fixes where Backdrop saves settings during installation?

jenlampton commented 7 years ago

We don't want to use the ugly array syntax by default.

The simper syntax was chosen because all people will understand it instantly. It's a universal syntax, and will benefit 100% of backdrop users. For the minority of sites that still need to use the array syntax for any of the various edge-cases, that Is an option too.

On Jun 7, 2017 10:18 AM, "Laryn - CEDC.org" notifications@github.com wrote:

@jenlampton https://github.com/jenlampton If we want it to use the array syntax by default on new installs (to avoid the special characters in the MySQL password causing a problem) we'd at least need a PR to change the default settings.php file, wouldn't we? And maybe some fixes where Backdrop saves settings during installation?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/backdrop/backdrop-issues/issues/2231#issuecomment-306864053, or mute the thread https://github.com/notifications/unsubscribe-auth/AAYSRxL2J6SSgrEuKRqzx8MiETXrlS2Jks5sBttsgaJpZM4KFxLV .

laryn commented 7 years ago

@jenlampton Can you think of another solution to the special character issue caused by the single line syntax? (ie. what this thread is about)

I don't think I'd call that an 'edge case' and expect new users who run into this to be able to figure out why they can't even install Backdrop in the first place. For example, using the cPanel "generate password" functionality on a new MySQL database user will almost always include a special character like this.

jenlampton commented 7 years ago

It's important that we avoid using a crazy backdrop-specific database connection syntax (by default) because that's the first thing people will see in settings.php. New users will see that first when setting up Backdrop, and that creates a poor first impression too.

Another thing we should do is make the installer UI be smarter about special characters in the database password. If a user enters one into the UI during installation, the installer should switch to using the crazy syntax when it writes to settings.php, or escape special characters when necessary. At the very least, it could throw an error or warning message letting people know what's up rather than just crashing the site. :(

Since what we're using now is a universal syntax, maybe we could look at what other all the other systems are doing about it, for reference?

laryn commented 7 years ago

I'm not sure what's so bad about the array format (I actually think it's clearer). How often will the 80% need to open settings.php anyway following a successful installation via the browser interface?

That said, I don't really have strong opinions as long as the issue gets solved.

klonos commented 7 years ago

Sorry @jenlampton, but I'm with the rest of the people on this one...

  1. I find that the array syntax is more self-explanatory than the URL syntax. Mainly because it uses placeholders for the parts in a way that resembles field label/input pairs. IMO this would help beginner devs more.
  2. The array syntax eliminates this problem here, which is a showstopper and causes a very bad UX for beginners trying to install Backdrop for the first time. Especially in very commonly-used environments like cPanel.
  3. "Backdrop values site builders over coders". Very few will need to directly mess with the settings.php file (which is code) in the first place. Or at least that's how things should be. Things are meant to be handled by our installer UI.

Now, if we can find an easy way to make the password field smart enough to detect and escape problematic characters before writing to the settings.php file, then I'm equally OK with that.

klonos commented 7 years ago

...to sum things up:

It's important that we avoid using a crazy backdrop-specific database connection syntax (by default)...

I agree.

...because that's the first thing people will see in settings.php.

These people will most likely be developers. Site builders do not mess with the settings.php file. At least beginner site builders that expect things to work from the UI.

New users will see that first when setting up Backdrop, and that creates a poor first impression too.

Not poorer impression than a "broken" installer that "doesn't work" and that leads you to have to fiddle with code.

docwilmot commented 7 years ago

Agreed. We need to either default the array syntax, or find/build an alternative to parse_url().

jenlampton commented 7 years ago

1) Top priority: If we're talking about the site-builder experience then the format we use in the settings.php file is irrelevant, since the site-builder won't be looking at it. We should fix the installer to either throw a validation error if a password is entered that won't work, or, ideally, deal properly with whatever password is provided. It's not a good idea to solve UX problems by creating DX problems when it's not necessary.

2) Our second priority is for the beginner coder (or beginner Backdrop user), and for them it's very important that we not use the array syntax from D7. I have a feeling all of you are too advanced to understand what it feels like for a non-developer to look at settings.php in Drupal 7 for the first time. I've trained hundreds of people on Drupal. The settings.php file shatters people's confidence - it deters them from thinking that they can do anything at all with Drupal's code.

I'm not sure what's so bad about the array format

I think we may all be missing the point here. Changing the syntax back to the mess from D7 is not the only answer.

If we want to fix the UX of the installer, good, let's do that! :)

If we want to improve the DX of the settings.php file, fine, let's do that too! But let's not move backwards. Let's find a new solution that doesn't have all the same problems that we've already solved.

Not poorer impression than a "broken" installer that "doesn't work" and that leads you to have to fiddle with code.

These two things are unrelated. We don't need to break the DX to fix the UX :) Let's fix all the things!

docwilmot commented 7 years ago

@jenlampton I think the 187 lines of documentation still exist; we just moved them to our docs site. :smile: Here's the current settings for dbases:

/**
 * Database configuration:
 *
 * Most sites can configure their database by entering the connection string
 * below. If using master/slave databases or multiple connections, see the
 * advanced database documentation at
 * https://api.backdropcms.org/database-configuration
 */
$database = 'mysql://user:pass@localhost/database_name';
$database_prefix = '';

Here's what it would look like:

/**
 * Database configuration:
 *
 * Most sites can configure their database by entering the connection details
 * below. If using master/slave databases or multiple connections, see the
 * advanced database documentation at
 * https://api.backdropcms.org/database-configuration
 */
  $database = array(
    'driver' => 'mysql',
    'database' => 'databasename',
    'username' => 'username',
    'password' => 'password',
    'host' => 'localhost',
    'prefix' => '',
  );

I dearsay I think the latter is certainly less scary.

In either case if we continue to use the string format, we must concede that users will have issues with password generators OR they will get nasty exceptions. Or we come up with an alternative to parse_url(). Frankly I'm not seeing many other options.

jenlampton commented 7 years ago

I think the 187 lines of documentation still exist; we just moved them to our docs site.

yes, because they aren't necessary anymore for people reading the settings.php file, thanks to our new cleaner syntax ;)

if we continue to use the string format, we must concede that users will have issues with password generators OR they will get nasty exceptions.

I gave two examples above of how this can be avoided...

Frankly I'm not seeing many other options.

Here's a third option, what WP does: https://codex.wordpress.org/Editing_wp-config.php

docwilmot commented 7 years ago

I gave two examples above of how this can be avoided

Sorry missing those, can you explain? Solutions are always a good thing.

jenlampton commented 7 years ago

Here's what it would look like:

/**
 * Database configuration:
 *
 * Most sites can configure their database by entering the connection details
 * below. If using master/slave databases or multiple connections, see the
 * advanced database documentation at
 * https://api.backdropcms.org/database-configuration
 */
  $database = array(
    'driver' => 'mysql',
    'database' => 'databasename',
    'username' => 'username',
    'password' => 'password',
    'host' => 'localhost',
    'prefix' => '',
  );

This example looks pretty clean and tidy... Does this work for backdrop already? Can we test it real quick?

jenlampton commented 7 years ago

Sorry missing those, can you explain?

If a user enters a password with special characters into the UI during installation, the installer should 1) switch to using the array syntax when it writes that info into settings.php, or 2) properly escape special characters (like slashes, using URI hex encodings) when necessary.

docwilmot commented 7 years ago

If a user enters a password with special characters into the UI during installation, the installer should 1) switch to using the array syntax when it writes that info into settings.php, or 2) properly escape special characters (like slashes) when necessary.

Makes sense

Actually, this looks pretty clean and tidy... Does this work for backdrop? Can we test it real quick?

Shouldnt work; would need a slight adjustment to bootstrap.inc.

jenlampton commented 7 years ago

Shouldn't work; would need a slight adjustment to bootstrap.inc.

I thought for a sec that was an alternate Drupal 7 syntax and I was like "How have I never seen this before?!?!" I think that is a good recommendation @docwilmot and I don't think new users will have trouble with that syntax :)

laryn commented 7 years ago

Looks good to me, @docwilmot!

olafgrabienski commented 7 years ago

Nice! @docwilmot, would you be willing to make a PR including the bootstrap.inc adjustment?

klonos commented 7 years ago

@jenlampton sorry, I understand now that when you say "scary" or "crazy" you are referring to the $databases['default']['default'] = part while I thought you were referring to the array element syntax. When I was saying "self-explanatory", I was referring to this part:

    'driver' => 'mysql',
    'database' => 'databasename',
    'username' => 'username',
    'password' => 'password',
    'host' => 'localhost',
    'prefix' => '',

So, basically yes, what @docwilmot suggests 😄

...or this:

If a user enters a password with special characters into the UI during installation, the installer should ... properly escape special characters (like slashes) when necessary.

...which is the perfect solution for both the installer problem and the better beginner DX when first facing settings.php.

jlfranklin commented 7 years ago

Will existing $databases['default']['default'] continue to work after the PR? Otherwise, this has the potential to break a site on upgrade.

jenlampton commented 7 years ago

Will existing $databases['default']['default'] continue to work after the PR? Otherwise, this has the potential to break a site on upgrade.

We should aim to keep the old syntaxes working too, if possible. We can mark them as deprecated and remove in 2.0 if we want.

docwilmot commented 7 years ago

If a user enters a password with special characters into the UI during installation, the installer should ... properly escape special characters (like slashes) when necessary. ...which is the perfect solution for both the installer problem and the better beginner DX when first facing settings.php.

This would still break if someone entered manually into settings.php using the string format. Its a reasonable option, but I dont like the idea that the start of an install could be that fragile. I think a simple array solves everyone's problems.

We'd need to ensure that all of

still work. We cant get rid of the complex one even in 2.0 unless we commit to removing slave db support. So at best we can deprecate the simple string now.

jenlampton commented 7 years ago

We cant get rid of the complex one even in 2.0 unless we commit to removing slave db support

...or rework how secondary databases are added, too. Either way that is certainly an edgecase. These other issues mentioned here are more important as they are likely to come up often. I like the new array syntax @docwilmot proposed, and if we start by replacing the string syntax and deprecating that I think it's still a win!

klonos commented 7 years ago

This would still break if someone entered manually into settings.php using the string format.

We could then add something along the lines of "the following characters need to be escaped" in the comments above the db connection line.

simone960 commented 6 years ago

I'm still encountering the issue when installing 1.8.0. I think this should be fixed whichever way needed. The beginners will get stuck right in the beginning when installing the backrop for the first time and need to google around, read the discussion and get the fix ==" I'm not a developer. What I fix is just to reset the password and removed those special character. It took me quite a while to get the site installed. This is really not impressive...

jenlampton commented 6 years ago

Thanks for your feedback @simone960. I agree, we should get this sorted out asap. Tagging for the 1.8.1 release.

Recapping the agreed-on solution for anyone working on a PR:

simone960 commented 6 years ago

Glad to hear that @jenlampton. Thanks for listening and making a decisive move. The door has to be first opened to people before we can talk about how great the interior design is.

jlfranklin commented 6 years ago

FYI, PHP handled this better up to 7.0.12 and 5.6.27.

https://3v4l.org/Urngf

jlfranklin commented 6 years ago

I think there are three things we should do, each can get its own issue:

olafgrabienski commented 6 years ago

@jlfranklin Wouldn't #2904 be sufficient to fix the actual bug?

jlfranklin commented 6 years ago

Only if you are entering the username and password for the database via Backrdrop's installer. If you type it directly into settings.php, then no, #2904 won't fix it.

olafgrabienski commented 6 years ago

@jlfranklin Thanks for your feedback. In the meantime I had a look at the PR for #2904 , looks good to me! Unfortunately I don't know how to help with the other two sub issues. If we don't fix them in time, I'd suggest however to merge #2904 as soon as possible because I assume that using Backdrop's installer is the default installation method for beginners.

quicksketch commented 6 years ago

I've merged @jlfranklin's recommendations in #2904 and #2914, so we should no longer have problems with the string format when installing via install.php.

I've always thought the string format was simpler/easier, but the change to PHP's parse_url() in new versions of PHP will really put a negative impact on the ease of use if URL-encoding is required. Encoding strings isn't something I would expect anyone to be able to do by hand, which is what the goal of the string format in the first place.

As we've largely fixed the bugs through @jlfranklin's PRs, let's retitle this to a DX feature request to switch to a simplified array syntax instead (if we want to go down that route).

yorkshire-pudding commented 1 year ago

Any ideas, what needs to happen where for the simplified array format to be supported in settings.php and settings.local.php? I've had to resort to the D7 method so I can put predefined credentials with complex passwords in for deploying to different environments.

yorkshire-pudding commented 11 months ago

This issue seems to be the cause of another issue in the forum - https://forum.backdropcms.org/forum/deployment-problems

olafgrabienski commented 11 months ago

Not sure if it was exactly this issue, but I also recently ran again in a situation as described in the forum: Wasn't able to get a site running on another server, and only because I remembered issues with the database password, I suggested to the hoster to please change the password, and then the site ran smoothly. This must be horrible for newcomers. If we can avoid such a situation by fixing this issue, so please let's fix it. (Happy to help, but don't know how.)

klonos commented 11 months ago

@olafgrabienski the technical direction here has been outlined in https://github.com/backdrop/backdrop-issues/issues/2231#issuecomment-341854268 and https://github.com/backdrop/backdrop-issues/issues/2231#issuecomment-341938604

Having re-read through the entire thread, I would sum things up to the following goals:

  1. we want to support the single-line string format: 
    $database = 'mysql://user:pass@localhost/database_name';
$database_prefix = '';

    Ideally, we'll add an inline comment in settings.php to be cautioning people editing that setting directly in the file that there are certain risks with using some special characters with that format.

  2. we also want to support a simplified array format: 
    $database = array(
 'driver' => 'mysql',
 'database' => 'databasename',
 'username' => 'username',
 'password' => 'password',
 'host' => 'localhost',
 'prefix' => '',
);
  3. we also want to keep supporting the advanced array format, which allows specifying multiple databases (notice the plural $databases instead of singular $database in the variable name): 
    $databases['default']['default'] = array(
 'driver' => 'mysql',
 'database' => 'databasename',
 'username' => 'username',
 'password' => 'password',
 'host' => 'localhost',
 'prefix' => '',
);
  4. we want to do the above without breaking backwards compatibility with settings copied over from D7 settings.php files, and with placing as many "safeguards" as possible, in order to prevent misconfigured db arrays or strings that cannot be parsed (some of these things are already in place with previous commits in other issues).
muranod commented 11 months ago

That was me with the deployment problems. I'm actually relieved to find out that it might have been the special characters in the database password that might have been causing my issues (over several days of trying to fix it). I'm not a programmer or developer - just building my own site from d7, d8, d9 experience. The array format would have been easier in my case, simply because of its clarity.