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

MigrationFailed on develop #148

Closed
luke- opened this issue Dec 21, 2023 · 9 comments
Closed

MigrationFailed on develop #148

luke- opened this issue Dec 21, 2023 · 9 comments
Labels
bug Something isn't working

Comments

@luke-
Copy link
Contributor

luke- commented Dec 21, 2023

Seems the recently introduce MigrateService is breaking the REST Tests:
humhub/humhub@4d142b0

See: https://github.com/humhub/rest/actions/runs/7235023663/job/19711995483

Also related to modules:
https://github.com/humhub/tasks/actions/runs/7287917971/job/19859512684

@luke- luke- added the bug Something isn't working label Dec 21, 2023
@luke-
Copy link
Contributor Author

luke- commented Dec 21, 2023

@martin-rueegg Any idea?

@martin-rueegg
Copy link

martin-rueegg commented Dec 21, 2023

Is it normal that the installation of the module has this Ø sign?

image

The test in the tasks module does not have that. Are you sure it is related?

@martin-rueegg
Copy link

@luke- how would I run those tests locally? Would I need to install the REST module?

@luke-
Copy link
Contributor Author

luke- commented Dec 21, 2023

The Checkout Rest Module Steps are required for modules, which implement an own REST API e.g. for tasks Module. The rest module itself doesn't need this. So these steps are skipped here.


I expect that the tests from the REST module can be executed like other tests.

The REST Module Tests seems to failing with:
image

So my first guess where the recent develop MigrationService changes.


If you want to execute the REST Tests of e.g. the tasks module, some additional steps are neccessary.
e.g. https://github.com/humhub/actions/blob/main/.github/workflows/module-tests-develop.yml#L152

@martin-rueegg
Copy link

It may be related to https://github.com/humhub/humhub/pull/6511/files#diff-46a3b644fa0848e9b9ff89b55e78b267182b69d39cea2f61870c54d37d0a78a5R413-R422

At least, that's another issue. Because if I search for "REST" in the marketplace (on my local installation) it fails.

@martin-rueegg
Copy link

Installation of the REST API module works locally. But then activation fails with the same error as in the test:

Invalid Configuration – [yii\base\InvalidConfigException](https://www.yiiframework.com/doc-2.0/yii-base-invalidconfigexception.html)
Migration failed!

    1. in /shared/httpd/humhub/htdocs/protected/humhub/services/MigrationService.php at line 265
    256257258259260261262263264265266267268269270271272273274

            }
     
            $this->trigger(self::EVENT_AFTER_MIGRATION, $result);
     
            /** @see \yii\console\controllers\BaseMigrateController::actionUp() */
            if ($result->result > ExitCode::OK) {
                $errorMessage = "Migration failed!";
     
                if (YII_DEBUG) {
                    throw new InvalidConfigException($errorMessage);
                }
     
                Yii::error($errorMessage, $this->module->id);
     
                return false;
            }
     
            return true;
        }

@martin-rueegg
Copy link

@luke- I might have found something in the MigrateController. But again, that's not the main issue here.

The following line causes an exception during migration:

$module->settings->set('enableJwtAuth', !empty($module->settings->get('jwtKey')));

Exception: Trying to get property 'settings' of non-object

which kinda makes sense since the module is not yet enabled:

$module = Yii::$app->getModule('rest');

I guess this has to do with

As previously, the module was temporarily "enabled" (for the Yii:$app->getModule() and the Yii::$app->moduleManager) and then disabled again. So this made the module available during migration.

So we should either

  • revert and enable the module first and disable it again upon migration errors
  • or adapt migrations accordingly.

@martin-rueegg
Copy link

Does not solve the problem yet, but improves error message:

Further adjustments possible as per my previous comment.

@luke-
Copy link
Contributor Author

luke- commented Dec 22, 2023

@martin-rueegg Thank you for your investigation.

For now, I have reintroduced the ModuleManager::disable() in case of error as you originally suggested.
humhub/humhub@7cd97f0

Background for Hotfix: All module tests run automatically at the weekend and I want to see whether develop is green now.

@luke- luke- closed this as completed Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants