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 changes #1344

Merged
merged 97 commits into from
Sep 16, 2018
Merged

Database changes #1344

merged 97 commits into from
Sep 16, 2018

Conversation

luceos
Copy link
Member

@luceos luceos commented Jan 11, 2018

@luceos luceos self-assigned this Jan 11, 2018
@luceos luceos changed the title WIP database changes (#1236) WIP database changes Jan 11, 2018
@franzliedke
Copy link
Contributor

Please request reviews when this is out of WIP. :)

@luceos
Copy link
Member Author

luceos commented Jan 11, 2018

Absolutely. Feel free to discuss anything that comes up in the meantime, might save some time refactoring it all over again 👍

@@ -18,7 +18,6 @@
$table->integer('post_id')->unsigned();
$table->integer('user_id')->unsigned();

$table->foreign('post_id')->references('id')->on('posts')->onDelete('cascade');
Copy link
Contributor

Choose a reason for hiding this comment

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

#1339 will change the posts table to InnoDB (d4850ec), so this can be re-added with a migration that has a more recent date/time.

Copy link
Member Author

@luceos luceos Jan 31, 2018

Choose a reason for hiding this comment

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

I think this migration is already newer than yours:

migrations/2018_01_11_120604_change_posts_table_to_innodb.php

I'll put it back.

@luceos luceos mentioned this pull request Jan 31, 2018
3 tasks
@franzliedke
Copy link
Contributor

Whew, quite big. If we get #1346 merged first, this one will be easier to review, right? (Still, this might need to be split up, to be honest.)

@luceos
Copy link
Member Author

luceos commented Apr 17, 2018

@flarum/core just to prevent confusion. I force pushed a clean version of the branch here, I'm basing it off of master. WIP.

@luceos luceos changed the title WIP database changes database changes Apr 24, 2018
@luceos
Copy link
Member Author

luceos commented Apr 24, 2018

Seems like most of the changes in core are done. Part of the referenced issue is done. Now need to take care of the extensions.

@tobyzerner tobyzerner mentioned this pull request Apr 24, 2018
11 tasks
@luceos luceos removed the request for review from franzliedke May 1, 2018 12:49
@luceos
Copy link
Member Author

luceos commented May 2, 2018

Waiting on #1418

@luceos luceos added this to the 0.1.0-beta.8 milestone May 2, 2018
@luceos luceos changed the title database changes [wip] database changes May 14, 2018
@tobyzerner
Copy link
Contributor

Update: have added model/JSON-API attribute renaming to this PR as it is completed project-wide. Awaiting feedback from @franzliedke on #1344 (comment) and @luceos testing table prefixes. In the meantime I will work on an indices PR.

@franzliedke
Copy link
Contributor

I have added a separate ticket for the table prefix thing, no need for that to block this PR.

@luceos
Copy link
Member Author

luceos commented Sep 11, 2018

Sorry guys, I currently have little time available. Noticed I was expected to run some tests with table prefixes.

@franzliedke
Copy link
Contributor

Table prefixes now have a separate PR.

This only needs resolving the migration thing now. 🙌

@tobyzerner
Copy link
Contributor

Which I will do by the end of the week :D

@tobyzerner
Copy link
Contributor

@franzliedke Please merge this (and the related PRs) if you are happy.

@franzliedke franzliedke merged commit 68afdd2 into master Sep 16, 2018
@franzliedke
Copy link
Contributor

@tobscure Should we remove the addPermissions migration shortcut then?

@franzliedke franzliedke deleted the 1236-database-changes branch September 16, 2018 19:00
@tobyzerner
Copy link
Contributor

@franzliedke No, because extensions can still safely use it – remember, their migrations need to be able to run on the most up-to-date Flarum database, so the addPermissions helper will always be appropriate.

@luceos
Copy link
Member Author

luceos commented Sep 17, 2018

😄 this got merged.. .. ..

.. ..

omg

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

Successfully merging this pull request may close these issues.

None yet

4 participants