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

Enh: run migrations manually #6711

Merged
merged 1 commit into from Dec 15, 2023

Conversation

martin-rueegg
Copy link
Contributor

@martin-rueegg martin-rueegg commented Dec 8, 2023

Currently, when navigating to Administration -> Information -> Database, any pending migration is applied immediately and leaves the administrator no choice of whether to actually apply them now.

  • This PR loads and displays the pending migration and gives the user the option to apply them.
  • Once migrations are applied, the user can reload the migration info (useful, if some migrations failed) and apply them again.
  • If no migration is applied, no option to apply them is offered.
  • If migrations are applied, cache is automatically cleared as well.

PR Admin

What kind of change does this PR introduce?

  • Feature

Does this PR introduce a breaking change?

  • No

The PR fulfills these requirements

  • It's submitted to the develop branch, not the master branch if no hotfix
  • All tests are passing
  • New/updated tests are included
  • Changelog was modified
  • A convincing reason for adding this feature (to avoid wasting your time, it's best to open a suggestion issue first and wait for approval before working on it)

@martin-rueegg
Copy link
Contributor Author

All failing tests have to do with

  • http://localhost/ vs
  • http://localhost:8080/

I have no clue how this could be related to the changes in this PR.

@martin-rueegg
Copy link
Contributor Author

martin-rueegg commented Dec 8, 2023

All failing tests have to do with

  • http://localhost/ vs
  • http://localhost:8080/

I have no clue how this could be related to the changes in this PR.

@luke- Could that have to do with your merges from master to develop?

Copy link
Contributor

@luke- luke- left a comment

Choose a reason for hiding this comment

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

@martin-rueegg Thanks, below my review.

@martin-rueegg
Copy link
Contributor Author

martin-rueegg commented Dec 14, 2023

@luke- Thank you for the review. I hope I have now been able to adjust accordingly.

@martin-rueegg martin-rueegg force-pushed the enh/run-migrations-manually branch 4 times, most recently from 99d38c3 to fbbc29c Compare December 14, 2023 21:59
@martin-rueegg martin-rueegg mentioned this pull request Dec 15, 2023
6 tasks
@luke- luke- added this pull request to the merge queue Dec 15, 2023
Merged via the queue into humhub:develop with commit 5d8795f Dec 15, 2023
6 checks passed
@martin-rueegg martin-rueegg deleted the enh/run-migrations-manually branch December 17, 2023 15:12
@yurabakhtin
Copy link
Contributor

@luke- @martin-rueegg Probably the bug is related somehow:

migration-bug

@yurabakhtin yurabakhtin self-assigned this Feb 8, 2024
@martin-rueegg
Copy link
Contributor Author

@yurabakhtin would you be able to describe the procedure?

Does the "not secure" info in the addressbar show up before or after you click "Update Database"?

I'd suggest that the problem may be a http vs https incongruency.

@yurabakhtin
Copy link
Contributor

@martin-rueegg I just started to work on new branch https://github.com/humhub/humhub/tree/enh/search where we have new migration m240203_112155_search so after run the update I got the errors, and even after several page refreshes I see the errors.

Does the "not secure" info in the addressbar show up before or after you click "Update Database"?

My local test server works on http://

local-site

I can reproduce the broken layout even with simple opening the url like http://humhub.local/admin/information/database?migrate=1 in new browser tab.
If you don't see this then don't spend your time, maybe it is only from my browser side, I will investigate this later deeply.
I just posted this here because it looks more related and in order to don't forget to fix this, thanks.

@martin-rueegg
Copy link
Contributor Author

Ah, I see. So I guess it has nothing to do with the "not secure" warning.
Also, looking at this PR I don't see how this could be the causing an error. Unless the MigrateController would somehow force https, rather than http in the up action.

I can't reproduce it here with https. And when I use http, it automatically switches to https (browser and server side). So I'd have to change my setup.

But you could print a screenshot of the requests sent (visible in the network tab of the developer console of the browser). I'm sure there are some css (and maybe other) files that are not correctly loaded.

@yurabakhtin
Copy link
Contributor

@martin-rueegg

  • /admin/information/database?migrate=1

files-404

strange why the files are not loaded, but if I copy their url into a new browser tab, I see the files are loaded correctly:

fontawesome
bootstrap

  • /admin/information/database

no-migrate-param

I see only one difference in the param v: v=1.16.0 vs v=1707498088

I have this bug in Chrome and FireFox browsers.

The branch master v1.15.3 has no such issue, only develop v1.16.0

@martin-rueegg
Copy link
Contributor Author

@yurabakhtin This is indeed very peculiar.

Would you mind pasting the source code of the two main pages here? I.e. the raw response from the following requests:

  • /admin/information/database?migrate=1
  • /admin/information/database

And do you have a chance to see if the requests actually hit the webserver?
Is the cache disabled in the chrome browser?

@yurabakhtin
Copy link
Contributor

yurabakhtin commented Mar 11, 2024

@martin-rueegg I have detected the problem was in the code Yii::$app->assetManager->clear() which is called from SettingController::flushCache(), on my side the code removes all folders from the folder /assets so it seems some of them cannot be recreated when page is loaded right after the clearing.
I have decided to redirect a page with storing a migration result in session, please check my solution in PR #6881.

After fix for me it looks like this:

fixed-db-migrate-run

@martin-rueegg
Copy link
Contributor Author

Thank you, @yurabakhtin. See my further comments in the respective PR.

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

3 participants