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

[WIP] Migrate properties to fileid #26949

Closed
wants to merge 2 commits into from
Closed

Conversation

VicDeo
Copy link
Member

@VicDeo VicDeo commented Jan 16, 2017

Description

Use migrations for all dav tables.
Use fileid instead of path in dav properties

Related Issue

Fixes #25563

Motivation and Context

t this table contains the full path to the target file and doesn't properly detect renames... so properties can get lost.
Furthermore, the column is too small and longer paths might not fit.

How Has This Been Tested?

Screenshots (if appropriate):

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@VicDeo VicDeo added this to the 10.0 milestone Jan 16, 2017
@mention-bot
Copy link

@VicDeo, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @tanghus and @bantu to be potential reviewers.

@VicDeo VicDeo force-pushed the migrate-properties-to-fileid branch from def21c1 to 78725ce Compare January 16, 2017 21:03
@VicDeo VicDeo force-pushed the migrate-properties-to-fileid branch from 78725ce to dd462a1 Compare January 17, 2017 17:19
// TODO: drop userid and propertypath
}

if (!$schema->hasTable("${prefix}addressbooks")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

oh, do we really also need these tables ? are they related to oc_properties ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@PVince81 oc_properties table belongs to dav app and is contained in core db schema by mistake now.
To use migrations on dav app all its schema needs to be ported to migrations first.
This is how I see it.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure if this is necessary. But maybe it's also ok to have it now, @DeepDiver1975 what do you think ?

@VicDeo make sure the DB dump diffs before and after are the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

@PVince81 ok, Let's assume we don't need all this tables. This table is in db_structure.xml and belongs to dav app.
Does this mean that I should rewrite entire core db structure to Migrations?
I think I can't use Migrations just for one table. So the choice either move the table to proper app (dav) and put use_migrations into its info.xml or do the same for the entire core.

Copy link
Contributor

Choose a reason for hiding this comment

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

A migration should be possible for just a single table.

Note that when enabling the app the first time, ownCloud will still setup the DB based on db_structure.xml. But updates have to go through migrations.

So if you setup that one table within the migration (and move it out of db_structure.xml), it should work.

*/
public function down(Schema $schema)
{
// We can't migrate below 10.0, can we?
Copy link
Contributor

Choose a reason for hiding this comment

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

no, let's always ignore the down part for now...

@VicDeo
Copy link
Member Author

VicDeo commented Jan 23, 2017

Closed in favor of #27005

@VicDeo VicDeo closed this Jan 23, 2017
@VicDeo VicDeo deleted the migrate-properties-to-fileid branch January 23, 2017 19:11
@lock
Copy link

lock bot commented Aug 4, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

oc_properties must use fileid instead of path
3 participants