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

Database naming changes #1236

Closed
7 of 11 tasks
tobyzerner opened this issue Sep 1, 2017 · 10 comments
Closed
7 of 11 tasks

Database naming changes #1236

tobyzerner opened this issue Sep 1, 2017 · 10 comments
Assignees
Milestone

Comments

@tobyzerner
Copy link
Contributor

tobyzerner commented Sep 1, 2017

As per https://gist.github.com/tobyzerner/b189580dd17b9def574f92692faa0e56


Edit by @luceos

  • update phpdoc
  • update existing properties in flarum/core where a renameColumn was executed Database changes #1344
  • update properties in core extensions where a renameColumn was executed
  • modify the accessor in User for the avatar_url c31c1ea
  • migrations for core; Database changes #1344
  • migrations for suspend ext
  • migrations for tags ext
  • add indices see comment by Toby
  • solve preferences drop from users, table or column
    • user notifications table
    • all other preferences to columns on users
@tobyzerner tobyzerner added this to the v0.1.0-beta.8 milestone Sep 1, 2017
@franzliedke
Copy link
Contributor

One more change: see c31c1ea.

franzliedke referenced this issue Sep 20, 2017
This is useful for forums integrating with an external website (eg. a
WordPress site), so they can reference existing avatars directly.

For alternative storage locations (eg. S3) the best practice will still
be to store a relative path and then configure an external base "assets
URL" (this is not currently possible - TODO).

Given this change, I think it would probably make sense to rename the
column to `avatar_url` in the upcoming batch of database naming changes
- then it can contain either a relative or an absolute URL -
@franzliedke do you agree?
@luceos luceos added Database and removed Backend labels Dec 19, 2017
@luceos luceos self-assigned this Jan 8, 2018
@luceos
Copy link
Member

luceos commented Jan 11, 2018

@tobscure the api-keys will have an allowed_ips column, but you didn't mention it being nullable. I can safely assume it should be nullable right?

@tobyzerner
Copy link
Contributor Author

yes :)

@luceos
Copy link
Member

luceos commented Jan 19, 2018

@flarum/core I suggested to @tobscure to change columns related to users to be postfixed with _by. A good example of this is:

Old: hide_user_id, new hidden_user_id, suggestion hidden_by.

Another question is why are we migrating columns to types that aren't supported by Laravel natively. This will limit our ability to support other drivers than MySQL in the future (even though Laravel supports several others). This is related to the settings table value column migrating to LONGBLOB which can only be done using a raw ALTER query.

Let me know what you think!

@luceos
Copy link
Member

luceos commented Jan 19, 2018

@tobscure can you explain to me what needs to be done with this:

image

@franzliedke
Copy link
Contributor

@luceos The _by postfix sounds sensible, is there any apparent convention in Toby's initial plan? If not, then go with it, I'd say.

As far as non-supported column types are concerned: if possible, we should avoid that, you are right. Can you list the columns (and their types) that use those strange types? (Or is that value column the only one?) Then we can decide on a case-by-case basis whether the benefits that those types give us may be worthy to keep.

@tobyzerner
Copy link
Contributor Author

tobyzerner commented Jan 24, 2018

I believe I didn't initially propose a _by suffix because I was trying to consolidate conventions, and the user relationship is already covered by the foreign key convention:

  • Foreign key -> {verbed}_{entity}_id (eg. hidden_user_id)
    • Verb can be omitted for primary relationship (eg. post author is just user_id)

_by on the other hand would be creating a special case for the users table, and makes the relationship with the users table non-explicit (it's not necessarily clear in the column name that the value of the column should be a user ID).

I'm not necessarily against this if you guys think it's better, just explaining the original reasoning.


The posts_users table is literally just those two columns. Other extensions (eg. likes) can add columns in the future.

Stick with blob for settings value then :)

@franzliedke
Copy link
Contributor

Very good reasoning, Toby. That "no special case for user IDs" part convinced me. 😉

@luceos
Copy link
Member

luceos commented Jan 25, 2018

Okay so:

  • don't do the _by
  • but do stick to existing laravel migrations builder methods
  • stick to blob for settings.value
  • posts_users comment related to extensibility can be ignored

Got it.

@luceos
Copy link
Member

luceos commented Feb 7, 2018

Waiting for #1339 to be merged so the foreign key can work again.

luceos added a commit that referenced this issue Mar 24, 2020
- split up deprecated and remaining user notification logic
- started building a test (needs work)
- created new Model for NotificationPreference
- created extender to register a NotificationChannel
- created extender to maintain UserPreferences

User preferences are still possible on the users table directly.
Registering a user preference allows for transformation to happen.
And provides easier accessors. Not sure we want this.

! tests need work.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants