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.

Sign up for GitHub

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

Jump to bottom

Translate all share exceptions #40164

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Translate all share exceptions #40164

wants to merge 1 commit into from

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Aug 30, 2023 •
edited
Loading

Summary

Fixes ambiguous messages like :
image

Checklist

@solracsf solracsf added the 3. to review Waiting for reviews label Aug 30, 2023
@solracsf solracsf added this to the Nextcloud 28 milestone Aug 30, 2023
@szaimen szaimen requested review from icewind1991, Valdnet, come-nc, a team and Fenn-CS and removed request for a team August 30, 2023 13:28
@solracsf solracsf added 2. developing Work in progress 3. to review Waiting for reviews and removed 3. to review Waiting for reviews 2. developing Work in progress labels Aug 30, 2023
@solracsf
Translate all share exceptions
7cf9cdd 
Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>

Fix drone

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>

Fix tests

Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
come-nc
come-nc requested changes Aug 31, 2023
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Why all the $message_t intermediate var, just passed the result of the call to the exception constructor directly?
Also why in some places you rename an existing var?

@solracsf
Copy link
Member Author

solracsf commented Aug 31, 2023 •
edited
Loading

The $message_t is already present in the codebase, so i've just applied the same logic. Example:

server/lib/private/Share20/Manager.php

Lines 361 to 362 in aba3f99

$message_t = $this->l->t('Files cannot be shared with delete permissions');
throw new GenericShareException($message_t);

Also why in some places you rename an existing var?

Where the existing $var has been renamed? Maybe a mistake?

@come-nc
Copy link
Contributor

come-nc commented Aug 31, 2023

Hum @nextcloud/files what do you think?

artonge
artonge requested changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@artonge artonge left a comment

Choose a reason for hiding this comment

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

Indeed, I would rather see the result used directly instead of having intermediate variables.

@come-nc where have you seen the variable rename ?

come-nc
come-nc reviewed Sep 14, 2023
View reviewed changes
lib/private/Share20/Manager.php
Comment on lines -437 to +456
$message = $this->l->n('Cannot set expiration date more than %n day in the future', 'Cannot set expiration date more than %n days in the future', $defaultExpireDays);
throw new GenericShareException($message, $message, 404);
$message_t = $this->l->n('Cannot set expiration date more than %n day in the future', 'Cannot set expiration date more than %n days in the future', $defaultExpireDays);
throw new GenericShareException($message_t, $message_t, 404);
Copy link
Contributor

Choose a reason for hiding this comment

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

@artonge Here (and a few other places below)

@solracsf
Copy link
Member Author

So, in fine, what's the status here?

@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
Merged
This was referenced Nov 6, 2023
Merged
Merged
This was referenced Nov 14, 2023
Merged
Merged
@blizzz blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023
@skjnldsv skjnldsv removed the 3. to review Waiting for reviews label Feb 23, 2024
@skjnldsv skjnldsv added the 2. developing Work in progress label Feb 23, 2024
@skjnldsv
Copy link
Member

So, in fine, what's the status here?

put the translation message straight into the Exception :)

Fenn-CS
Fenn-CS reviewed Feb 27, 2024
View reviewed changes
Copy link
Contributor

@Fenn-CS Fenn-CS left a comment

Choose a reason for hiding this comment

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

According to previous reviews, there seems to be consensus to avoid using intermediate $message or $message_t variables, can this be updated?

@solracsf
Copy link
Member Author

Will do as soon as I find some time, no problem. 👍

This was referenced Mar 12, 2024
Merged
Merged
Merged
This was referenced Mar 20, 2024
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Mar 28, 2024
Merged
81 tasks
@skjnldsv skjnldsv modified the milestones: Nextcloud 29, Nextcloud 30 Mar 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fenn-CS Fenn-CS Fenn-CS left review comments

@come-nc come-nc come-nc requested changes

@artonge artonge artonge requested changes

@icewind1991 icewind1991 Awaiting requested review from icewind1991

@Valdnet Valdnet Awaiting requested review from Valdnet

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
2. developing Work in progress bug feature: language l10n and translations
Projects
None yet
Milestone
Nextcloud 30
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@solracsf @come-nc @skjnldsv @artonge @Fenn-CS @joshtrichards @blizzz