Skip to content

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented Oct 14, 2024

Fix nextcloud/files_pdfviewer#1191

This is needed for the PDF viewer localisation since it requests a locale.properties file.

@szaimen szaimen added bug 3. to review Waiting for reviews labels Oct 14, 2024
@szaimen szaimen added this to the Nextcloud 31 milestone Oct 14, 2024
@szaimen
Copy link
Contributor Author

szaimen commented Oct 14, 2024

/backport to stable30

@szaimen
Copy link
Contributor Author

szaimen commented Oct 14, 2024

/backport to stable29

Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

Could you add a little mention in the commit message that this is needed for PDF viewer localisation? Thanks!

This is needed for the PDF viewer localisation since it requests a `locale.properties` file.

Signed-off-by: Simon L. <szaimen@e.mail.de>
@szaimen szaimen force-pushed the enh/noid/fix-properties-files branch from b4b56ce to d4b0309 Compare October 14, 2024 13:12
@szaimen
Copy link
Contributor Author

szaimen commented Oct 14, 2024

Could you add a little mention in the commit message that this is needed for PDF viewer localisation? Thanks!

done

@joshtrichards
Copy link
Member

Haven't looked too closely yet, but off-hand this type of change suggests there was perhaps also a breaking change introduced at some point, no? Because we'll need this in our Admin Manual's nginx config too...

@nickvergessen
Copy link
Member

Haven't looked too closely yet, but off-hand this type of change suggests there was perhaps also a breaking change introduced at some point, no?

Yeah well mostlikely when the upstream library changed it's way of handling things

Because we'll need this in our Admin Manual's nginx config too...

Yes, please add similarly to nextcloud/documentation#12273

@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label Oct 14, 2024
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

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

As agreed with @szaimen I will take over, as there are additional changes needed for the PDF viewer. Blocking to avoid merging until I can push them.

@susnux
Copy link
Contributor

susnux commented Oct 14, 2024

I wonder why this is working using just the default Apache htaccess? (currently it works for me without any changes).

But in general maybe this should be changed to the Apache equivalent of try_files for apps/xxx/js/...?

@szaimen
Copy link
Contributor Author

szaimen commented Oct 14, 2024

I wonder why this is working using just the default Apache htaccess? (currently it works for me without any changes).

I suppose you did not enable pretty urls? (they are active for AIO by default)...

$content .= "\n RewriteRule ^core/js/oc.js$ index.php [PT,E=PATH_INFO:$1]";
$content .= "\n RewriteRule ^core/preview.png$ index.php [PT,E=PATH_INFO:$1]";
$content .= "\n RewriteCond %{REQUEST_FILENAME} !\\.(css|js|mjs|svg|gif|ico|jpg|jpeg|png|webp|html|otf|ttf|woff2?|map|webm|mp4|mp3|ogg|wav|flac|wasm|tflite)$";
$content .= "\n RewriteCond %{REQUEST_FILENAME} !\\.(css|js|mjs|svg|gif|ico|jpg|jpeg|png|webp|html|otf|ttf|woff2?|map|webm|mp4|mp3|ogg|wav|flac|wasm|tflite|properties)$";
Copy link
Member

Choose a reason for hiding this comment

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

This feels dangerous to me. There could be other .properties files that should rather not get exposed by accident. Can you instead add a pattern that matches the exact file name only?

Copy link
Member

Choose a reason for hiding this comment

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

The properties files come from PDF.js locales; there is a main locale.properties and one XXX/viewer.properties for each locale, where XXX is the locale code.

Does something like this would be enough or you would prefer something even stricter?:

$content .= "\n  RewriteCond %{REQUEST_FILENAME} !pdfjs/.*/(locale|viewer)\\.properties$";

Copy link
Member

Choose a reason for hiding this comment

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

Looks good 👍

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 29, 2024
@blizzz blizzz mentioned this pull request Jan 8, 2025
This was referenced Jan 14, 2025
This was referenced Jan 21, 2025
@blizzz blizzz mentioned this pull request Jan 29, 2025
1 task
@blizzz blizzz modified the milestones: Nextcloud 31, Nextcloud 32 Jan 29, 2025
This was referenced Aug 22, 2025
This was referenced Sep 2, 2025
This was referenced Sep 25, 2025
@skjnldsv skjnldsv modified the milestones: Nextcloud 32, Nextcloud 33 Sep 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2. developing Work in progress backport-request bug pending documentation This pull request needs an associated documentation update

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Files_pdfviewer : localisation doesn't work with NC 29.0.7

8 participants