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.

Sign up for GitHub

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

Jump to bottom

Fix base locale #10308

Merged
merged 2 commits into from
Apr 19, 2019
Merged

Fix base locale #10308

merged 2 commits into from
Apr 19, 2019

Conversation

igormukhingmailcom
Copy link
Contributor

@igormukhingmailcom igormukhingmailcom commented Apr 14, 2019 •
edited
Loading

Q A
Branch? 1.3
Bug fix? yes
New feature? no
BC breaks? not sure (it backward compatible on default fixtures configuration)
Deprecations? not sure (deprecation messages was added, but not appear on default fixtures configuration)
Related tickets fixes #10307, partially #7346
License MIT

So, in BC in mind, with this PR it now will work like this:

  • If you have default (empty) configuration, as was before:

    sylius_fixtures:
    # ...
    fixtures:
        locale: ~

    Then, base locale will be added to configuration.

  • But if you provided a list of locales:

    sylius_fixtures:
    suites:
        my_custom_suite_to_add_to_existing_database:
            listeners:
                orm_purger: false
            fixtures:
                locale:
                    options:
                        locales:
                            - "de_DE"

    Then, base locale will NOT be added to configuration.

Deprecation message was added to avoid usage base locale at all in future. To suppress that message on default configuration, "%locale%" was passed directly to the locales option at src/Sylius/Bundle/CoreBundle/Resources/config/app/fixtures.yml.

TODO

  • Define Sylius version where base locale will be removed OR remove deprecation message if base locale should be kept by default (when fixture defined like locale: ~) - I'm not 100% sure about this so want to discuss this first.

@igormukhingmailcom igormukhingmailcom requested a review from a team as a code owner April 14, 2019 12:50
@GSadee GSadee added the Potential Bug Potential bugs or bugfixes, that needs to be reproduced. label Apr 15, 2019
@GSadee GSadee changed the base branch from master to 1.3 April 19, 2019 10:24
@GSadee
Copy link
Member

GSadee commented Apr 19, 2019

The base of this pull-request was changed, you need fetch and reset your local branch
if you want to add new commits to this pull request. Reset before you pull, else commits
may become messed-up.

Unless you added new commits (to this branch) locally that you did not push yet,
execute git fetch origin && git reset "fix-base-locale" to update your local branch.

Feel free to ask for assistance when you get stuck 👍

@lchrusciel lchrusciel merged commit 41f0cbb into Sylius:1.3 Apr 19, 2019
@lchrusciel
Copy link
Member

Thank you, Igor! 🎉

@pamil
Copy link
Contributor

pamil commented Apr 29, 2019

This is actually a BC break and should be reverted, we need to figure out a solution which allows you not to load the default locale and prevents throwing an error if the default locale is defined explicitly.

I used to do a project, in which fixtures would break after this change because we removed the default locale from the explicit list but still wanted it to be created.

@pamil pamil mentioned this pull request Apr 29, 2019
Merged
@pamil
Copy link
Contributor

pamil commented Apr 29, 2019

Created #10341 which reverts it.

@pamil
Copy link
Contributor

pamil commented Apr 29, 2019

Idea by @lchrusciel:

Add a boolean option load_default_locale (or prepend_default_locale) which is true by default. Disabling it will stop LocaleFixture from adding the default locale.

lchrusciel added a commit that referenced this pull request Apr 29, 2019
@lchrusciel
bug #10341 Revert "Fix base locale" (pamil)
7614a13 
This PR was merged into the 1.3 branch.

Discussion
----------

Reverts #10308, see the reasoning in the comment there.

Commits
-------

d6444c5 Revert "Fix base locale"
@lchrusciel
Copy link
Member

Ref. #10342

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lchrusciel lchrusciel lchrusciel approved these changes

Assignees
No one assigned
Labels
Potential Bug Potential bugs or bugfixes, that needs to be reproduced.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@igormukhingmailcom @GSadee @lchrusciel @pamil