Page MenuHomeDevCentral
Create Task
Maniphest T999

NotifyNewCommitsToDiffusion fails to check if we've or not a Phabricator instance for this project
Closed, ResolvedPublic
Actions

Description

291a59a6 introduced exceptions in PhabricatorAPI static constructors. Formerly, they returned null.

When a new payload comes from GitHub, the NotifyNewCommitsToDiffusion job notifies Phabricator of the new commit. But when there is no Phabricator instance for the same door (or in the future a mapping, see T851), the code doesn't detect this new exception.

This error occurs in our production notifications.nasqueron.org instance:

[2017-01-07 06:14:10] production.ERROR: RuntimeException: No credentials for Phabricator project Keruald. in /var/wwwroot/default/app/Phabricator/PhabricatorAP
I.php:67
Stack trace:
#0 /var/wwwroot/default/app/Phabricator/PhabricatorAPIFactory.php(26): Nasqueron\Notifications\Phabricator\PhabricatorAPI::forProject('Keruald')
#1 /var/wwwroot/default/bootstrap/cache/compiled.php(6325): Nasqueron\Notifications\Phabricator\PhabricatorAPIFactory->getForProject('Keruald')
#2 /var/wwwroot/default/app/Jobs/NotifyNewCommitsToDiffusion.php(146): Illuminate\Support\Facades\Facade::__callStatic('getForProject', Array)
#3 /var/wwwroot/default/app/Jobs/NotifyNewCommitsToDiffusion.php(136): Nasqueron\Notifications\Jobs\NotifyNewCommitsToDiffusion->fetchAPI()
#4 /var/wwwroot/default/app/Jobs/NotifyNewCommitsToDiffusion.php(75): Nasqueron\Notifications\Jobs\NotifyNewCommitsToDiffusion->fetchRequirements()
#5 /var/wwwroot/default/app/Listeners/PhabricatorListener.php(40): Nasqueron\Notifications\Jobs\NotifyNewCommitsToDiffusion->handle()
#6 /var/wwwroot/default/app/Listeners/PhabricatorListener.php(26): Nasqueron\Notifications\Listeners\PhabricatorListener->notifyNewCommits(Object(Nasqueron\Not
ifications\Events\GitHubPayloadEvent))
NOTE: tests for NotifyNewCommitsToDiffusion are needed to detect this kind of issue during refactoring.

Revisions and Commits

rNOTIF Notifications center
D807rNOTIF6653ad57b95c Improve API replies handling in NotifyNewCommitsToDiffusion

Related Objects

Event Timeline

dereckson created this task.Aug 31 2016, 17:14
dereckson merged a task: T998: NotifyNewCommitsToDiffusion fails to check if we've or not a Phabricator instance for this project.
dereckson updated the task description. (Show Details)Sep 11 2016, 01:37
dereckson raised the priority of this task from Low to High.Jan 7 2017, 17:44
dereckson merged a task: T1089: NotifyNewCommitsToDiffusion: RuntimeException: No credentials for Phabricator project ....
dereckson updated the task description. (Show Details)
dereckson added a project: Regression.
dereckson added a comment.Jan 7 2017, 17:46
In T1089#13198, @dereckson wrote:

I can repro the regression with the following test:

tests/Jobs/NotifyNewCommitsToDiffusionTest.php
public function testJobWhenThereIsNoPhabricatorInstanceForTheProject () {
    $this->mockJob("not-existing-project");
    $this->job->handle();
}

protected function mockJob(string $project = "acme") {
    $this->job = new NotifyNewCommitsToDiffusion(
        $project,
        "ssh://acme/k2.git"
    );
}

This produces the following stack trace:

Nasqueron\Notifications\Tests\Jobs\NotifyNewCommitsToDiffusionTest::testJobWhenThereIsNoPhabricatorInstanceForTheProject
RuntimeException: No credentials for Phabricator project not-existing-project.

/app/notifications/app/Phabricator/PhabricatorAPI.php:67
/app/notifications/app/Phabricator/PhabricatorAPIFactory.php:26
/app/notifications/vendor/laravel/framework/src/Illuminate/Support/Facades/Facade.php:217
/app/notifications/app/Jobs/NotifyNewCommitsToDiffusion.php:146
/app/notifications/app/Jobs/NotifyNewCommitsToDiffusion.php:136
/app/notifications/app/Jobs/NotifyNewCommitsToDiffusion.php:75
/app/notifications/tests/Jobs/NotifyNewCommitsToDiffusionTest.php:37
dereckson moved this task from Bugs to Working on on the Notifications center board.Jan 7 2017, 17:49
dereckson added a revision: D807: Improve API replies handling in NotifyNewCommitsToDiffusion.Jan 7 2017, 19:35
dereckson closed this task as Resolved by committing rNOTIF6653ad57b95c: Improve API replies handling in NotifyNewCommitsToDiffusion.Jan 7 2017, 20:35
dereckson added a commit: rNOTIF6653ad57b95c: Improve API replies handling in NotifyNewCommitsToDiffusion.
Nasqueron DevCentral · If it had been much bigger the moon would have had a core of ice. · Powered by Phabricator