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

use $schemaClosure in migration #2985

Closed
wants to merge 1 commit into from
Closed

Conversation

notEvil
Copy link

@notEvil notEvil commented Jul 9, 2023

fixes #2980

Please note that I'm not particularly familiar with php and the codebase in general. The solution is a proposal.

@github-actions github-actions bot added the stale label Jul 11, 2023
@dartcafe
Copy link
Collaborator

Thanks. But should it not be enough to return the schema, used by the migration method?

@notEvil
Copy link
Author

notEvil commented Jul 11, 2023

No, because in the context of convert-type it is has to operate on a different database. During migration, the class gets instantiated only once, remembering the source schema (say SQLite) in the process. However, it then needs to create the tables on the target schema (say Postgres) using the target connection. So it needs to delegate the actual execution to Nextcloud.

@github-actions github-actions bot removed the stale label Jul 12, 2023
@dartcafe dartcafe self-requested a review July 12, 2023 20:14
Copy link
Collaborator

@dartcafe dartcafe left a comment

Choose a reason for hiding this comment

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

Sorry, my time is again limited, but now I had the time to look over it and check the aftermath of it. And find out there should none, since createTables is the only method called inside the migration atm, this should work and not break the commands.

It wasn't a good idea anyways to build this up with separate schemas for the tableManager and the indexManager, but I have this on my to do list for later.

New migrations according to the app version are preferred over changing migration methods, but in this particular situation I am sure it is ok.

TLDNR: LGTM

@dartcafe
Copy link
Collaborator

Thanks for contribution!!

@dartcafe
Copy link
Collaborator

Please note that I'm not particularly familiar with php and the codebase in general.

Fun fact. I am not even a programmer and had no clue of PHP or JS when I started contributing here .

@dartcafe dartcafe added this to the 5.2 milestone Jul 12, 2023
@notEvil
Copy link
Author

notEvil commented Jul 12, 2023

Interesting :)

Regarding the failed check:
https://github.com/nextcloud/server/blob/e9ac0287f2b7d4da856bec416a282c8fc4c7feab/lib/private/DB/MigrationService.php#L444C3-L444C3
passes a OC\DB\SchemaWrapper but declares it as OCP\DB\ISchemaWrapper. And my quick and dirty solution was to not care about the type. This is where someone with more experience immediately knows what to do about it, closing the circle.

@github-actions github-actions bot added the stale label Jul 14, 2023
@dartcafe dartcafe modified the milestones: 5.2, 5.3 Jul 15, 2023
@dartcafe dartcafe removed the stale label Jul 20, 2023
@dartcafe
Copy link
Collaborator

Erks. I think, we need another approach. I will come back to it later. Currently I'm rather busy.

@github-actions github-actions bot added the stale label Jul 23, 2023
@github-actions github-actions bot closed this Jul 29, 2023
@dartcafe dartcafe removed the stale label Jul 29, 2023
@dartcafe dartcafe reopened this Jul 29, 2023
@github-actions github-actions bot added the stale label Jul 31, 2023
@github-actions github-actions bot closed this Aug 5, 2023
@dartcafe dartcafe reopened this Aug 7, 2023
@dartcafe dartcafe removed the stale label Aug 7, 2023
@github-actions github-actions bot added the stale label Aug 9, 2023
@dartcafe dartcafe removed the stale label Aug 9, 2023
@dartcafe
Copy link
Collaborator

Superseded by #3024

@dartcafe dartcafe closed this Aug 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App doesn't cooperate with occ db:convert-type
2 participants