Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Issue #4443 - Zend\I18n\Translator\Loader\PhpArray can now load files from include path #4515

Closed
wants to merge 7 commits into from

Conversation

robertmarsal
Copy link
Contributor

Added possibility to load translation files from the include path.
Issue #4443

@@ -29,7 +29,8 @@ class PhpArray implements FileLoaderInterface
*/
public function load($locale, $filename)
{
if (!is_file($filename) || !is_readable($filename)) {
if (!stream_resolve_include_path($filename) &&
Copy link
Member

Choose a reason for hiding this comment

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

please hold the result of stream_resolve_include_path within an own variable and (if not false) use this variable further checks and for include itself

@robertmarsal
Copy link
Contributor Author

@marc-mabe Done. Thanks for the feedback.

@@ -29,14 +29,15 @@ class PhpArray implements FileLoaderInterface
*/
public function load($locale, $filename)
{
if (!is_file($filename) || !is_readable($filename)) {
$fromIncludePath = stream_resolve_include_path($filename);
if (!$fromIncludePath && (!is_file($filename) || !is_readable($filename))) {
Copy link
Member

Choose a reason for hiding this comment

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

stream_resolve_include_path() simply resolves to first found file using the configured include_path. If no relative path will be given this will be used without include_path. It doesn't check the type (see stream_resolve_include_path('/tmp')) or readability (see stream_resolve_include_path('/root'))

So please use $fromIncludePath:

if (!$fromIncludePath || !is_file($fromIncludePath) || !is_readable($fromIncludePath)) {
         throw new Exception\InvalidArgumentException(sprintf(
             'Could not find or open file %s for reading',
             $filename
         )); 
}
$messages = include $fromIncludePath;

@DASPRiD
Copy link
Member

DASPRiD commented May 21, 2013

One question: what exactly is the use case for this?

@bacinsky
Copy link
Contributor

The use case is, when you can't use absolute or relative path in the module config file to load (Zend) PHP translations, e.g. your module is located somewhere outside of the application but wants to load translations from other libraries in the include path. And the exception lied, because the file can be opened for reading.

@DASPRiD
Copy link
Member

DASPRiD commented May 22, 2013

This looks kinda inconsistent to me, as the other loaders are not able to look in the same paths.

@robertmarsal
Copy link
Contributor Author

Should I add the same feature for the other loaders?

@weierophinney
Copy link
Member

@robertboloc Yes, please.

@robertmarsal
Copy link
Contributor Author

Done

@bacinsky
Copy link
Contributor

The last issue is, that now it can't load from PHAR, because stream_resolve_include_path() returns false when $filename starts with phar://

@robertmarsal
Copy link
Contributor Author

Updated the code to allow loading from phar.

weierophinney added a commit that referenced this pull request Jun 28, 2013
Issue #4443 - Zend\I18n\Translator\Loader\PhpArray can now load files from include path
weierophinney added a commit that referenced this pull request Jun 28, 2013
@ghost ghost assigned weierophinney Jun 28, 2013
@weierophinney
Copy link
Member

Merged to develop for release with 2.3.0.

@Ocramius
Copy link
Member

@weierophinney can this be reverted? We don't use the include path at all in ZF2, we shouldn't start now (nor should modify the environment, which is a thing we're all proud of)

@weierophinney
Copy link
Member

@Ocramius The developer of this and the config one made some good arguments for having the capabilities, which largely center around non-code-related file resolution. While I agree that one should stay away from the include_path, the facilities only provide overhead in specific situations, and are basically opt-in.

weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015
weierophinney added a commit to zendframework/zend-i18n that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants