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

chore: accounts controller patch response #10234

Merged
merged 1 commit into from
Oct 9, 2024

Conversation

st3iny
Copy link
Member

@st3iny st3iny commented Oct 8, 2024

Does not really change anything because the response is not used anywhere in the frontend (hence a chore type commit).

PATCH https://localhost/index.php/apps/mail/api/accounts/2000

Before

{
    "id": 2000
}

After

{
    "id": 2000,
    "accountId": 2000,
    "name": "Alice",
    "order": 5,
    [...]
}

@kesselb
Copy link
Contributor

kesselb commented Oct 8, 2024

Nice catch!

The other methods return an Account object (that implements JsonSerializable and calls $this->mailAccount->toJson underneath) .

I think it's more correct to also return an account object here, even if the result stays the same ;)

Something like

		$dbAccount = $this->accountService->save($dbAccount);

		return new JSONResponse(
			new Account($dbAccount)
		);

@kesselb
Copy link
Contributor

kesselb commented Oct 8, 2024

PHP Fatal error: Declaration of Psr\Log\AbstractLogger::emergency($message, array $context = []) must be compatible with Psr\Log\LoggerInterface::emergency(Stringable|string $message, array $context = []): void in /home/runner/actions-runner/_work/mail/mail/nextcloud/apps/mail/vendor/psr/log/Psr/Log/AbstractLogger.php on line 22

Unrelated, I will take a look.

@st3iny
Copy link
Member Author

st3iny commented Oct 8, 2024

I think it's more correct to also return an account object here, even if the result stays the same ;)

Goo point, I'll amend the PR.


Unrelated, I will take a look.

Thanks, this is also happening on my local dev instance.

@st3iny st3iny force-pushed the chore/accounts-controller-patch branch from 09e7519 to b4d526e Compare October 9, 2024 05:59
@kesselb kesselb force-pushed the chore/accounts-controller-patch branch from b4d526e to 7675a9a Compare October 9, 2024 11:22
@kesselb kesselb enabled auto-merge October 9, 2024 11:31
@kesselb kesselb merged commit 6a3cd01 into main Oct 9, 2024
35 checks passed
@kesselb kesselb deleted the chore/accounts-controller-patch branch October 9, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants