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

When the connection to database fails, the API should return a valid response #8537

Merged
merged 2 commits into from Aug 21, 2015

Conversation

tsteur
Copy link
Member

@tsteur tsteur commented Aug 10, 2015

fixes #7903

Whenever the error handler now gets an error, and an API was called, we will still output a valid API message unless an exception contains an HTML message. In this case it will just display the HTML message as it is usually supposed to be in a nice format etc. Not sure re any possible side effects. I tested maintenance mode, and some other errors and they were still displayed correctly.

@tsteur tsteur added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Aug 10, 2015

$response = str_replace("\n", "", $response);

$this->assertStringStartsWith('<?xml version="1.0" encoding="utf-8" ?><result> <error message=', $response);
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't compare whole output since I'm kinda certain that MySQL will report messages differently depending on the used version

@tsteur tsteur added this to the 2.15.0 milestone Aug 10, 2015
$testingEnvironment = new \Piwik\Tests\Framework\TestingEnvironmentVariables();
$testingEnvironment->configFileLocal = PIWIK_INCLUDE_PATH . '/plugins/Installation/tests/resources/config.ini.php';
$testingEnvironment->save();
}
Copy link
Member

Choose a reason for hiding this comment

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

NOTE: It should also be possible to set configFileLocal by overriding the provideContainerConfig(), eg:

public function provideContainerConfig()
{
    return array(
        'test.vars.configFileLocal' => PIWIK_INCLUDE_PATH . '/plugins/...';
    );
}

This isn't documented, but is intended to eventually replace direct TestingEnvironmentVariables() and TestingEnvironmentVariables::save() use. It's not really important, but if you'd like, you can use provideContainerConfig() instead here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I just tried it but didn't work... is it also possible to specify the $testEnvironment->configOverride this way?

Copy link
Member

Choose a reason for hiding this comment

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

Should be possible to set configOverride this way. configFileLocal doesn't work because it's created before the DI container (since the INI config is needed before this point)... but there should ideally be only one way to set these properties...

Anyway, it's not important so I will merge this PR.

@diosmosis
Copy link
Member

Looks good to me. Not merging directly since I left a comment, but you can merge yourself whenever.

diosmosis added a commit that referenced this pull request Aug 21, 2015
When the connection to database fails, the API should return a valid response w/ correct output format (ie JSON, XML, whatever) instead of just HTML.
@diosmosis diosmosis merged commit 05b6c82 into master Aug 21, 2015
@diosmosis diosmosis deleted the 7903 branch August 21, 2015 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants