Skip to content

Properly inject EventDispatched in BackgroundRepair - #14805

Merged
rullzer merged 1 commit into
masterfrom
fix/backgroundrepair
Mar 23, 2019
Merged

Properly inject EventDispatched in BackgroundRepair#14805
rullzer merged 1 commit into
masterfrom
fix/backgroundrepair

Conversation

@rullzer

Copy link
Copy Markdown
Member

Else it will just be null when called

Signed-off-by: Roeland Jago Douma roeland@famdouma.nl

Else it will just be null when called
Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

@ChristophWurstChristophWurst left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sure

@ChristophWurstChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Mar 22, 2019
/**
* @param EventDispatcherInterface $dispatcher
*/
public function setDispatcher(EventDispatcherInterface $dispatcher): void {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If this is only executed in tests, why did it work then in the actual code?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It is then actually needed in line 94. 🤔

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's only used in

$queue->add('OC\Migration\BackgroundRepair', [
'app' => $appId,
'step' => $step]);
so I guess it just always ran without events

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Because

if (!is_null($this->dispatcher)) {
$this->dispatcher->dispatch("$scope::$method",
newGenericEvent("$scope::$method", $arguments));
}
checks existence before using it

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But shouldn't then this fail?

publicfunction__construct(array$repairSteps, EventDispatcherInterface$dispatcher) {

because there is only a type hint and not an explicit = null. Or is this only causing issues in strict mode?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there already a PR, because I can't find any.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

This is it. We now properly inject it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But where? 🤔

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

image

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was referencing the initialization of a new BackgroundRepair instance. But you are right - they are added to the queue and the queue then uses the DI container. Sorry for the noise.

@faily-bot

faily-botBot commented Mar 22, 2019

Copy link
Copy Markdown

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 17256: failure

TESTS=acceptance, TESTS-ACCEPTANCE=app-files-sharing-link

  • tests/acceptance/features/app-files-sharing-link.feature:128
Show full log
 Scenario: access a shared link protected by password with an invalid password # /drone/src/github.com/nextcloud/server/tests/acceptance/features/app-files-sharing-link.feature:128
Given I act as John # ActorContext::iActAs()
And I am logged in # LoginPageContext::iAmLoggedIn()
And I share the link for "welcome.txt" protected by the password "abcdef" # FilesAppSharingContext::iShareTheLinkForProtectedByThePassword()
And I write down the shared link # FilesAppSharingContext::iWriteDownTheSharedLink()
Element is no longer attached to the DOM
For documentation on this error, please visit: http://seleniumhq.org/exceptions/stale_element_reference.html
Build info: version: '2.53.1', revision: 'a36b8b1', time: '2016-06-30 17:37:03'
System info: host: 'aae1d19bb3de', ip: '172.17.0.8', os.name: 'Linux', os.arch: 'amd64', os.version: '4.4.0-141-generic', java.version: '1.8.0_91'
Driver info: driver.version: unknown (WebDriver\Exception\StaleElementReference)
When I act as Jane # ActorContext::iActAs()
And I visit the shared link I wrote down # PublicShareContext::iVisitTheSharedLinkIWroteDown()
And I authenticate with password "fedcba" # PublicShareContext::iAuthenticateWithPassword()
Then I see that the current page is the Authenticate page for the shared link I wrote down # PublicShareContext::iSeeThatTheCurrentPageIsTheAuthenticatePageForTheSharedLinkIWroteDown()
And I see that a wrong password for the shared file message is shown # PublicShareContext::iSeeThatAWrongPasswordForTheSharedFileMessageIsShown()

@rullzer
rullzer merged commit d94b56d into masterMar 23, 2019
@rullzer
rullzer deleted the fix/backgroundrepair branch March 23, 2019 07:24
@rullzerrullzer mentioned this pull request Mar 26, 2019
9 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4. to releaseReady to be released and/or waiting for tests to finishbug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@rullzer@nickvergessen@MorrisJobke@ChristophWurst@kesselb