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

[DependencyInjection] Added option `ignore_errors: not_found` for imported config files #31310

Merged
merged 1 commit into from Nov 8, 2019

Conversation

@pulzarraider
Copy link
Contributor

pulzarraider commented Apr 28, 2019

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

If someone want to add optional config file. The only available choice was to add ignore_errors: true option

e.g.

imports:
    - { resource: parameters.yml, ignore_errors: true }

But this will hide all errors in imported file. We ran in many situations that broke our Symfony applications because we had a typo in this imported files.

This PR introduce new possible value not_found for ignore_errors option. It can be used for optional config files like the ignore_errors: true, but it will ignore only the file non-existence, not the possible syntax errors inside.

Usage:

imports:
    - { resource: parameters.yml, ignore_errors: not_found}
@pulzarraider pulzarraider force-pushed the pulzarraider:config_ignore_not_found branch from 3d94650 to 5ab59f2 Apr 28, 2019
@nicolas-grekas nicolas-grekas added this to the next milestone Apr 29, 2019
Copy link
Member

nicolas-grekas left a comment

Hi, thanks for the PR, it looks like a good idea to me.
About the implementation, I made my review into a patch, see pulzarraider#1

@pulzarraider pulzarraider changed the title [Config] [DependencyInjection] [Routing] Added option `ignore_not_found` for imported config files WIP [Config] [DependencyInjection] [Routing] Added option `ignore_not_found` for imported config files Apr 30, 2019
@pulzarraider pulzarraider changed the title WIP [Config] [DependencyInjection] [Routing] Added option `ignore_not_found` for imported config files [WIP] [Config] [DependencyInjection] [Routing] Added option `ignore_not_found` for imported config files Apr 30, 2019
@pulzarraider pulzarraider force-pushed the pulzarraider:config_ignore_not_found branch 6 times, most recently from c7916cf to 9c7fc7b Apr 30, 2019
@pulzarraider pulzarraider force-pushed the pulzarraider:config_ignore_not_found branch 3 times, most recently from 6632ba8 to 8ecb133 Jun 3, 2019
@pulzarraider pulzarraider changed the base branch from master to 4.4 Jun 3, 2019
@pulzarraider pulzarraider changed the base branch from 4.4 to master Jun 3, 2019
@pulzarraider pulzarraider force-pushed the pulzarraider:config_ignore_not_found branch from 8ecb133 to ecc0e7e Jun 3, 2019
@pulzarraider pulzarraider changed the title [WIP] [Config] [DependencyInjection] [Routing] Added option `ignore_not_found` for imported config files [Config] [DependencyInjection] [Routing] Added option `ignore_not_found` for imported config files Jun 3, 2019
@pulzarraider

This comment has been minimized.

Copy link
Contributor Author

pulzarraider commented Jun 3, 2019

Thank you @nicolas-grekas, I've merged your MR and did some more small improvements.

@OskarStark

This comment has been minimized.

Copy link
Contributor

OskarStark commented Jun 3, 2019

@pulzarraider If I am correct, this must now go into 4.4 branch, could you please switch the target branch for this PR?

I already did this for the corresponding Docs-PR symfony/symfony-docs#11647

@pulzarraider pulzarraider force-pushed the pulzarraider:config_ignore_not_found branch from dc1478b to 6c02b52 Sep 27, 2019
@pulzarraider pulzarraider changed the base branch from master to 4.4 Sep 27, 2019
@pulzarraider pulzarraider force-pushed the pulzarraider:config_ignore_not_found branch 5 times, most recently from dee7d76 to cddf68c Sep 28, 2019
@pulzarraider pulzarraider requested review from nicolas-grekas and stof Sep 28, 2019
@pulzarraider

This comment has been minimized.

Copy link
Contributor Author

pulzarraider commented Sep 28, 2019

I think this feature is finished. @nicolas-grekas @stof @sstok please review it again.
I hope it will be added in sf 4.4.

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.4 Oct 27, 2019
@nicolas-grekas nicolas-grekas changed the title [Config] [DependencyInjection] [Routing] Added option `ignore_not_found` for imported config files [DependencyInjection] Added option `ignore_errors: not_found` for imported config files Nov 7, 2019
@nicolas-grekas nicolas-grekas force-pushed the pulzarraider:config_ignore_not_found branch 3 times, most recently from 18c039e to 419a9b5 Nov 7, 2019
Copy link
Member

nicolas-grekas left a comment

I've rebased the PR and changed the logic to make it local to the DI component only.

(fabbot failure is unrelated - patch to be applied globally starting from 3.4)

…mporting config files
@nicolas-grekas nicolas-grekas force-pushed the pulzarraider:config_ignore_not_found branch from 419a9b5 to e0ee01c Nov 7, 2019
@pulzarraider

This comment has been minimized.

Copy link
Contributor Author

pulzarraider commented Nov 8, 2019

Thank you @nicolas-grekas for your changes.

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Nov 8, 2019

Thank you @pulzarraider.

nicolas-grekas added a commit that referenced this pull request Nov 8, 2019
…_found` for imported config files (pulzarraider)

This PR was merged into the 4.4 branch.

Discussion
----------

[DependencyInjection] Added option `ignore_errors: not_found` for imported config files

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

If someone want to add optional config file. The only available choice was to add `ignore_errors: true` option

e.g.
```
imports:
    - { resource: parameters.yml, ignore_errors: true }
```

But this will hide all errors in imported file. We ran in many situations that broke our Symfony applications because we had a typo in this imported files.

This PR introduce new possible value `not_found` for `ignore_errors` option. It can be used for optional config files like the `ignore_errors: true`, but it will ignore only the file non-existence, not the possible syntax errors inside.

Usage:
```
imports:
    - { resource: parameters.yml, ignore_errors: not_found}
```

Commits
-------

e0ee01c [DependencyInjection] Added option `ignore_errors: not_found` while importing config files
@nicolas-grekas nicolas-grekas merged commit e0ee01c into symfony:4.4 Nov 8, 2019
2 of 3 checks passed
2 of 3 checks passed
fabbot.io Some changes should be done to comply with our standards.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pulzarraider pulzarraider deleted the pulzarraider:config_ignore_not_found branch Nov 8, 2019
This was referenced Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.