Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
File loaders can now set the type to load explicitly#7353
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
xabbuh commentedJan 11, 2017
👍 for the changes, but we should document the version this feature was added to |
ogizanagi left a comment• edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
The changes in the dependency injection component doc look perfect as is, but I think it misses animport example, as in the original issue:
# app/config/config.ymlimports: -{ resource: my_custom_config.ext, type: yml }
Is there any suitable place where it can be added?
components/config/resources.rst Outdated
| ..tip:: | ||
| If your application uses unconventional file extensions (for example, your | ||
| YAML files have a ``.res`` extension) you can pass the file type as the |
ogizanagiJan 11, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Actually, it depends on the loader implementation. The one described in this part of the documentation (YamlUserLoader code above) totally ignores thetype argument, as was done before by the DIC loaders 😅 (I think it should not appear at all in the Config component doc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are there plans to add this feature to these loaders too ... or it doesn't make sense? Thanks!
ogizanagiJan 12, 2017 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The only other loaders implementingLoaderInterface from the Config component are the routing ones.
They handle the$type argument quite differently:
# Symfony\Component\Routing\Loader\YamlFileLoader/** * {@inheritdoc} */publicfunctionsupports($resource,$type =null){returnis_string($resource) &&in_array(pathinfo($resource,PATHINFO_EXTENSION),array('yml','yaml'),true) && (!$type ||'yaml' ===$type);}
The type cannot be forced, but should be empty or match the extension. Maybe it can be updated to match the new behavior introduced in the di loaders.
But I still think this note should not appear in the Config component documentation, as it should described theLoaderInterface and what it is used for. It should not document implementations from other components.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 thanks for the explanation.
components/config/resources.rst Outdated
| The YamlUserLoader will be used to load this resource, | ||
| // YamlUserLoader is used to load this resource because it supports | ||
| // files with the '.yml' extension | ||
| since it supports files with a "yml" extension |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This line should be removed, right?
ogizanagi commentedJan 12, 2017
Oops! Looks like I forgot something here: Status: Reviewed |
xabbuh commentedFeb 28, 2017
Thank you Javier. |
Thisfixes#7352.
I propose to document this in a subtle manner because this is rarely needed by users.
Ping@ogizanagi because he was the original author of the related PR. Thanks!