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

YamlFileLoader does 'supports' based purely on file extension #20308

Closed
salvocanna opened this issue Oct 26, 2016 · 8 comments

Comments

Projects
None yet
6 participants
@salvocanna
Copy link

commented Oct 26, 2016

After playing a while with the symfony configuration I realised there's no way to make symfony parse and load a yaml file if its extension is not in ['yml', 'yaml']

Shouldn't it be a configuration?

Something like:

imports:
    - { resource: my_custom_config.ext, format: yml }

I can submit a pull request

I'm sorry if this may be already implemented somehow and I'm not aware of it.

Cheers

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Oct 26, 2016

Extending the supports method of the yaml loader should be enough, right?

@chalasr

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

We should not have to add anything as there is already a type key e.g:

imports:
    - { resource: my_custom_config.ext, type: yml }

Unfortunately, this key is not used at all in the DI YamlFileLoader. The supports() method looks like:

public function supports($resource, $type = null)
{
    return is_string($resxource) && in_array(pathinfo($resource, PATHINFO_EXTENSION), array('yml', 'yaml'), true));
}

Imho we should change it to something like:

public function supports($resource, $type = null)
{
    return is_string($resxource) && (in_array(pathinfo($resource, PATHINFO_EXTENSION), array('yml', 'yaml'), true) || in_array($type, array('yml', 'yaml'), true));
}

So we don't care about the resource extension except if type is not provided, same for other file loaders. WDYT?

@salvocanna

This comment has been minimized.

Copy link
Author

commented Oct 27, 2016

@chalasr exactly also, in the same loader, parseImports($content, $file):

$this->import($import['resource'], null, isset($import['ignore_errors']) ? (bool) $import['ignore_errors'] : false, $file);

should become something like

$this->import($import['resource'], isset($import['type']) ? $import['type'] : null, isset($import['ignore_errors']) ? (bool) $import['ignore_errors'] : false, $file);

as the second parameter is the type.

I think that's all.

@chalasr

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

Good catch @salvocanna, shall I propose the change or do you want to (that's your issue, you deserve it)?

@salvocanna

This comment has been minimized.

Copy link
Author

commented Oct 27, 2016

@chalasr thanks! I'd like to do it even if it's the first one ever made (if I'm successful..)

Which branch should it go to?

I currently use symfony 2.8 so would be nice to see it there, but depends, should it be seen as a feature or bug fix?

@GuilhemN

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2016

I'd go for a feature as it never worked before but others may think otherwise.

@chalasr

This comment has been minimized.

Copy link
Member

commented Oct 27, 2016

@salvocanna I would propose it as a bugfix, as the type key exists and does not look useful as is. At worst your PR can be merged on another branch than the one you target at first (you can also change the base yourself from github if someone ask it).

So if you go for a bugfix then 2.7 should be targeted, see http://symfony.com/doc/current/contributing/code/patches.html#step-3-submit-your-patch for details and don't forget to update/add the corresponding tests :)

@ogizanagi

This comment has been minimized.

Copy link
Member

commented Nov 23, 2016

See #20611

fabpot added a commit that referenced this issue Jan 10, 2017

feature #20611 [DI] FileLoaders: Allow to explicit type to load (ogiz…
…anagi)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] FileLoaders: Allow to explicit type to load

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #20308
| License       | MIT
| Doc PR        | Not yet

(fabbot will scream out regarding the PR fixtures)

Commits
-------

6b660c2 [DI] FileLoaders: Allow to explicit type to load

symfony-splitter pushed a commit to symfony/dependency-injection that referenced this issue Jan 10, 2017

feature #20611 [DI] FileLoaders: Allow to explicit type to load (ogiz…
…anagi)

This PR was merged into the 3.3-dev branch.

Discussion
----------

[DI] FileLoaders: Allow to explicit type to load

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony#20308
| License       | MIT
| Doc PR        | Not yet

(fabbot will scream out regarding the PR fixtures)

Commits
-------

6b660c2114 [DI] FileLoaders: Allow to explicit type to load

@fabpot fabpot closed this Jan 10, 2017

@fabpot fabpot reopened this Jan 10, 2017

@fabpot fabpot closed this Jan 10, 2017

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