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

replaced language setting in APPPATH."Language" #2355

Closed

Conversation

SunilEver
Copy link

Fix issue #2354

Description
Updated CodeIgniter\Language\Language::requireFile() to prefer APPPATH."Language" Directory

Checklist:

  • Securely signed commits
  • Component(s) with PHPdocs
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@MGatner
Copy link
Member

MGatner commented Oct 21, 2019

I like the idea but this is causing testPrioritizedLocator() to fail, which makes me believe that the priority load order still isn't working as expected.

@jim-parry
Copy link
Contributor

Some problems with this PR:

  • commits not GPG-signed
  • unit testing not updated
  • user guide not updated

@jim-parry
Copy link
Contributor

@MGatner @lonnieezell Neither the user guide or code suggest anything about prioritizing one namespace over another, which appears to be the goal of the bug report / PR. Should App be prioritized?

@MGatner
Copy link
Member

MGatner commented Oct 21, 2019

I think file locating needs to have an explicit hierarchy, which might vary depending on the component. FileLocator already uses the generic ordering: System, Config, App, Modules. The issue here is that Language->requireFile() essentially reverses the order, which could be misleading to anyone familiar with the regular hierarchy:

$files   = Services::locator()->search($path);
...
$strings = array_replace_recursive(...$strings);

As a regular module writer, I do think it is important for developers to be able to supply their own phrasing without modifying the module - so in this case I'm in favor of prioritizing App but maybe by pushing this to FileLocator instead of the override method here.

@jim-parry
Copy link
Contributor

So, classes etc order would be System, Config, App, Modules, but message collapsing should be System, Config, Modules, App?
The Language class might be more appropriate for it, as that would disrupt things elsewhere the least.

@MGatner
Copy link
Member

MGatner commented Dec 10, 2019

I do think what this is addressing constitutes an issue, but I don't think this is the solution. lang() is a very high priority function as international-supported projects may call it hundreds of times in a single load, and this solution adds an additional filesystem search to every call:

Services::locator()->locateFile($path)

Any language files in APPPATH will already be included in Services::locator()->search($path), it might just need to handle prioritizing a bit differently.

Additionally, this is causing tests to fail and the commit is not signed. Closing the PR, but I will open a corresponding issue so it doesn't get lost.

@nControl88
Copy link

but I will open a corresponding issue so it doesn't get lost

Just had a look at the open issues, and no corresponding issue found. Just a reminder to prevent this issue being unresolved.

@MGatner
Copy link
Member

MGatner commented Mar 17, 2020

The issue is linked directly above yours and has already been addressed. If you’re having a similar issue you could open a support thread on the forums.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants