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

fix: cannot install without db prefix #4001

Merged
merged 1 commit into from
Sep 20, 2024
Merged

fix: cannot install without db prefix #4001

merged 1 commit into from
Sep 20, 2024

Conversation

luceos
Copy link
Member

@luceos luceos commented Jun 17, 2024

Database prefix and database password can be empty.

Caused by the rewrite in 6f11e04, as a consequence some settings of the database config are now incorrectly enforced/required.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered?
  • For core PRs, does this need to be in core, or could it be in an extension?
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
  • Core developer confirmed locally this works as intended.
  • Tests have been added, or are not appropriate here.

Required changes:

  • Related documentation PR: (Remove if irrelevant)
  • Related core extension PRs: (Remove if irrelevant)

@luceos luceos requested a review from SychO9 June 17, 2024 22:05
@luceos luceos requested a review from a team as a code owner June 17, 2024 22:05
Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Allowing no prefix makes sense, are we allowing no password for socket connections?

@luceos
Copy link
Member Author

luceos commented Jun 18, 2024

Allowing no prefix makes sense, are we allowing no password for socket connections?

You can use no password, on development and production. Whether it's advisable has to be seen. Regardless of that decision, a user will receive errors based on their credentials and the credentials the database server expects anyway.. But blatantly demanding a password would potentially block developers with a local dev environment where root has no password.

Ref: MYSQL_ALLOW_EMPTY_PASSWORD on https://hub.docker.com/_/mysql for instance.

Copy link
Member

@askvortsov1 askvortsov1 left a comment

Choose a reason for hiding this comment

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

Thanks!

@SychO9
Copy link
Member

SychO9 commented Jun 18, 2024

Don't merge yet please. Till the big db ones are merged

@luceos
Copy link
Member Author

luceos commented Aug 3, 2024

Ref:

Both merged, will rebase and see if the issue persisted.

@luceos luceos force-pushed the dk/fix-install-20240618 branch from 6474db0 to 5593db9 Compare August 3, 2024 15:02
@luceos
Copy link
Member Author

luceos commented Aug 3, 2024

@SychO9 seems one of your PR's made password already optional, I assume sqlite because that's completely file based. So the only thing this PR now fixes is:

a) that prefixes are optional
b) that without a prefix no "preg_replace cannot use null as argument"-error is thrown

PS force pushed because I just made the changes on top of the merged/updated 2.x branch instead of rebasing.

@SychO9 SychO9 merged commit d3144ee into 2.x Sep 20, 2024
26 of 28 checks passed
@SychO9 SychO9 deleted the dk/fix-install-20240618 branch September 20, 2024 07:31
@SychO9 SychO9 added this to the 2.0 milestone Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants