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

[WIP] dismiss translation files of extensions not installed #1954

Closed
wants to merge 1 commit into from

Conversation

@luceos
Copy link
Member

luceos commented Dec 9, 2019

Fixes #1837

Changes proposed in this pull request:

If I'm not mistaken, this PR is an easy implementation to ignore translation files for extensions not installed. It will however re-read them every time the translation cache files are updated. In addition one can still force files into the translator if one has to.

Reviewers should focus on:

Is this okay?

Confirmed

  • Frontend changes: tested on a local Flarum installation.
  • Backend changes: tests are green (run composer test).
…not installed will be dismissed

if ($file->isFile()
&& in_array($file->getExtension(), ['yml', 'yaml'])
&& (strstr($slug, '-') || $extensions->isEnabled($slug))) {

This comment has been minimized.

Copy link
@rob006

rob006 Dec 22, 2019

What is the purpose of strstr($slug, '-') here?

This comment has been minimized.

Copy link
@datitisev

datitisev Dec 22, 2019

Member

To transform the package name (vendor/name) into an extension ID (vendor-name).

E.g. flarum/auth-github into flarum-auth-github. This is the same extension ID system that is used when putting extensions in JS flarum.extensions (they are all IDs).

This extension ID is then used to see if the locale has been loaded (keys that look like vendor-name.admin) - this is a breaking change as extensions that do not follow this would not have their locale loaded, but... it's necessary for performance improvements.

We might want to make a method for it though, instead of simply repeating the same code without explaining what it does well.

Hopefully that made sense 🙂.

This comment has been minimized.

Copy link
@rob006

rob006 Dec 22, 2019

Hopefully that made sense slightly_smiling_face.

Sorry, it does not make sense for me at all. :P strstr() does not make any replacement, it returns substring after first -, which then will be cast to boolean. Essentially it will be interpreted as true for every extension (since they must contain at least one - for separating vendor and package name), which is both cryptic (strpos() !== 0 seems to be more common and efficient way for searching if string contains another string) and incorrect (it no longer matters if extension is enabled).

This comment has been minimized.

Copy link
@datitisev

datitisev Dec 22, 2019

Member

Oh, I didn't even look at the context of the code, I apologize. I see what you were asking about now.

Yeah, I guess we should be using strpos here instead.

If you only want to determine if a particular needle occurs within haystack, use the faster and less memory intensive function strpos() instead.

This comment has been minimized.

Copy link
@luceos

luceos Dec 23, 2019

Author Member

I think this code misses the negate. Because this conditional attempts to verify the file name is not core.yml or validation.yml.

This comment has been minimized.

Copy link
@rob006

rob006 Dec 23, 2019

Then why you just don't use in_array() to check this? Using strstr() is harder to understand and less reliable (I can name a file as myextensions.yml and it also always be loaded).

This comment has been minimized.

Copy link
@luceos

luceos Dec 23, 2019

Author Member

What array would you use in this in_array conditional @rob006 ?

This comment has been minimized.

Copy link
@rob006

rob006 Dec 23, 2019

@luceos core and validation?

This comment has been minimized.

Copy link
@luceos

luceos Dec 23, 2019

Author Member

Yeah also possible. The reason I decided against that is because we agreed to demand filenames equal to the extension shortened identifier (removing flarum-ext-). This code would allow additional files for core. Not sure what we prefer here.

This comment has been minimized.

Copy link
@rob006

rob006 Dec 23, 2019

This code would allow additional files for core.

IMO these files should be explicitly whitelisted (this list could be exposed as public constant). Relying on - existence in file name allows language pack to use any name for always loaded translations, which may conflict with new conventions added in future. For example in future Flarum could use extend.yml file loaded in specific condition, but such file could be already used by some extensions (because Flarum will always load it, so why not?).

@luceos luceos changed the title dismiss translation files of extensions not installed [WIP] dismiss translation files of extensions not installed Dec 23, 2019
@luceos luceos closed this Jan 8, 2020
@datitisev

This comment has been minimized.

Copy link
Member

datitisev commented Jan 17, 2020

Closed because we may want to approach this issue differently.

@rob006

This comment has been minimized.

Copy link

rob006 commented Jan 18, 2020

@datitisev @luceos Any details about this "different approach"? Solution from this PR looks pretty fine, I was hoping that #1837 will be fixed in next beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.