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

[Settings] Missing parameters issues in twig templates and settings loading #3269

Merged
merged 1 commit into from Dec 14, 2015
Merged

Conversation

aramalipoor
Copy link
Contributor

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets no
License MIT
Doc PR -

There are a lot of times and loading a settings schema fails because we had a previous deprecated value in the database which we didn't want anymore and since it's not easy to create migrations for settings schema's I thought maybe it's safe to ignore missing options ONLY when loading from database.

@pjedrzejewski
Copy link
Member

@aramalipoor Why closing this? :)

@aramalipoor
Copy link
Contributor Author

@pjedrzejewski After some thoughts it seemed a little odd to me and also there were many more situations, in which OptionsResolver's restrictions (e.g. validateOptionsExistence) is going to be annoying.

This PR is only covering unknowns parameters situations where we can easily check if it is not known then remove it.
BUT the problem is that what about new proposed parameters (say in a new version of schema) that are marked as required, OptionsResolver does not provide us a way to get a list of required parameters or a way to resolve without check for required params.

All in all I thought that this PR is incomplete and let's look for a better solution.

@aramalipoor aramalipoor reopened this Sep 14, 2015
@aramalipoor aramalipoor changed the title [SettingsBundle] Ignore missing options when loading a settings schema [Settings] Missing parameters issues in twig templates and migration situations Sep 14, 2015
@aramalipoor aramalipoor changed the title [Settings] Missing parameters issues in twig templates and migration situations [Settings] Missing parameters issues in twig templates and settings loading Sep 14, 2015
@pjedrzejewski pjedrzejewski added this to the v0.16.0 milestone Sep 15, 2015
@aramalipoor
Copy link
Contributor Author

@pjedrzejewski This PR is going to solve half of the problem but it seems that we really needs this half! Is it possible for you to review and merge it? Thanks ;)

@pjedrzejewski
Copy link
Member

@aramalipoor Could you rebase it with master? I will merge it then. 👍

@pjedrzejewski pjedrzejewski removed this from the v0.16.0 milestone Dec 14, 2015
pjedrzejewski pushed a commit that referenced this pull request Dec 14, 2015
[Settings] Missing parameters issues in twig templates and settings loading
@pjedrzejewski pjedrzejewski merged commit 174ed5a into Sylius:master Dec 14, 2015
@pjedrzejewski
Copy link
Member

Thank you Aram!

@aramalipoor aramalipoor deleted the fix/settings-unknown-params branch December 15, 2015 09:07
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.

2 participants