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

Add command to change system language #24

Conversation

JoshuaBehrens
Copy link
Contributor

Now you can run and get an administration using German as system language. Useful for German only shops that don't force the users to enter English data first. Can be applied to any other locale that shopware ships.

bin/console system:install --create-database --basic-setup --locale=de-DE

Lots of it is 📋🍝 from recovery.

@mitelg
Copy link
Member

mitelg commented Mar 26, 2020

hey @JoshuaBehrens
could you please rebase your branch against master?

@JoshuaBehrens JoshuaBehrens force-pushed the feature/command-to-set-system-locale branch from 46a4d61 to 21c8093 Compare March 26, 2020 11:13
@JoshuaBehrens JoshuaBehrens changed the base branch from 6.1 to master March 26, 2020 11:13
@JoshuaBehrens
Copy link
Contributor Author

Feels like a simple rebase is not the solution :D

@JoshuaBehrens JoshuaBehrens force-pushed the feature/command-to-set-system-locale branch from 21c8093 to d502e0f Compare March 26, 2020 11:53

private function getLocaleId(string $iso): string
{
$stmt = $this->connection->prepare('SELECT locale.id FROM locale WHERE LOWER(locale.code) = LOWER(?)');
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can use like. it's faster.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A like is case insensitive? ... When I just read this. I could've input the argument lowered first 😅 If I use like I have to escape the right-hand-side.

And if we change something here we have to change it in shopware/recovery as well.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usage of function on the left side is slow, because it can't use the index.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, having a function on the left is slow. Good thing it is not used very frequent. But does like really help here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is case insenstive only if the column uses a _ci collation which I think is the case with Shopware columns.
In that case = actually already does the trick. If necessary one can do something like code = 'de-de' COLLATE utf8mb4_unicode_ci to ensure case insensitive column comparison is used. But as mentioned I think Shopware creates columns case insensitive, so it should not be necessary.

@CLAassistant
Copy link

CLAassistant commented Jun 8, 2020

CLA assistant check
All committers have signed the CLA.

$output->section('Shop locale');

$locale = $input->getArgument('locale');
$this->setDefaultLanguage($locale);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you definetly have to clear the cache after you changed the system language,
the whole entity cache is stale at this point leading to numerous ugly bugs 😆

Been there, done that 🤣

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do suggest to clear the cache? I thought I've done that here. I must admit I am a bit out of the topic by now :D

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've done that when the command get's called as part of the system install command, not if it is run by itself.

You can inject the \Shopware\Core\Framework\Adapter\Cache\CacheClearer into the service and use the clear() method.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @JoshuaBehrens, could you take care of the comment from @keulinho? 🙂

@keulinho
Copy link
Contributor

keulinho commented Jul 15, 2020

Additionally this can never fully work after the fact:
E.g. we want to switch the default language from englisch to spanish.
After we ran this command every translation in english that is already in the system will actually be in spanish and vice versa, as the translated values are not migrated between the languages.

We should clearly state that shortcomming in the description of the command and maybe even prefix that command with dev: or danger: so it doesn't get executed lightly in a prod env.

It totally makes sense to add the locale option to the system:install command, adding a command in general that allows changing the system default language at any time needs some more consideration.

@JoshuaBehrens
Copy link
Contributor Author

Hey @keulinho I am on it shopware/shopware#860 with the guys from wexo for the command that can be run anytime you want. That is capable of changing every translation at runtime. I run 5 or 6 installations with this pull request and German as default language. At the time of this pull request the boost days were far away. I just need this command to easily integrate it into the setup command.

@JoshuaBehrens JoshuaBehrens force-pushed the feature/command-to-set-system-locale branch from d502e0f to 35c24c3 Compare August 3, 2020 01:01
@JoshuaBehrens
Copy link
Contributor Author

@keulinho I did some changes to prevent unintended execution. With this lock the cache will get cleared by the followed command in any case so that is not a problem either anymore. My locking might be just a bit... exotic :)

@shyim
Copy link
Member

shyim commented Aug 26, 2020

Language creation is missing when it's not existing by default
Fix: https://github.com/FriendsOfShopware/production/commit/ff7ea09967c08d8cec336c84bce0077b9714c2d9

@AndreasA
Copy link
Contributor

What about currency etc.? Also why not use the ShopService from Recovery for this? might require it to be moved elsewhere?

@AndreasA
Copy link
Contributor

Without adjusting the runCommands method like this, it fails for me on the second cache clear, as that one requires the command argument - probably a Symfony bug but this is an easy workaround:

  /**
     * @param array<string, array<string, string>> $commands
     *
     * @return int
     */
    private function runCommands(array $commands, OutputInterface $output): int
    {
        foreach ($commands as $parameters) {
            $output->writeln('');

            $command = $this->getApplication()->find($parameters['command']);
            if (!$command->getDefinition()->hasArgument('command')) {
                unset($parameters['command']);
            }
            $returnCode = $command->run(new ArrayInput($parameters, $command->getDefinition()), $output);
            if ($returnCode !== 0) {
                return $returnCode;
            }
        }

        return 0;
    }

@JoshuaBehrens
Copy link
Contributor Author

@AndreasA this is a problem of this repository. There is already a pull request solving this. I like your approach to exit early when a command fails.

@AndreasA
Copy link
Contributor

AndreasA commented Oct 21, 2020

OK. Thanks for the informatin.

One other thing. In the shopService the swap language command does not disable foreign key checks which is important as otherwise, e.g. the mail template contents will not be switched.

It is done like this:

    private function swapDefaultLanguageId(string $newLanguageId): void
    {
        $stmt = $this->connection->prepare(
            'UPDATE language
             SET id = :newId
             WHERE id = :oldId'
        );

        // assign new uuid to old DEFAULT
        $stmt->execute(
            [
                'newId' => Uuid::randomBytes(),
                'oldId' => Uuid::fromHexToBytes(Defaults::LANGUAGE_SYSTEM),
            ]
        );

        // change id to DEFAULT
        $stmt->execute(
            [
                'newId' => Uuid::fromHexToBytes(Defaults::LANGUAGE_SYSTEM),
                'oldId' => $newLanguageId,
            ]
        );
    }


private function swapDefaultLanguageId(string $newLanguageId): void
{
$this->connection->exec('SET FOREIGN_KEY_CHECKS = 0');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as mentioned in comments, in recovery's ShopService they are not disabled which is a better solution as CASCADE UPDATE takes care of all necessary changes and this way, the content of mail templates etc. is correctly assigned.

Even better would be to re-assign everything correctly afterwards but...

@AndreasA
Copy link
Contributor

AndreasA commented Oct 21, 2020

One more question: In case the system default language is part of a language pack. How would one ensure the language pack is loaded prior to executing the system default change command? Although I guess it does not matter if it is installed afterwards?
Or I would need to install it by modifying the command to activate prior to switching languages? Hmm.. OK for now e.g. the English templates would be used then. would be nice to determine base language for templates - not specific to this command and also to install language packs prior to setting system default which should also setup the mail templates accordingly.

@J-Rahe
Copy link
Contributor

J-Rahe commented Dec 1, 2020

This PR was closed due to inactivity. If this change is still important to you, feel free to create a new pull request.
For more information about our contribution guidelines, see https://docs.shopware.com/en/shopware-platform-dev-en/contribution/contribution-guideline

@J-Rahe J-Rahe closed this Dec 1, 2020
@philipreinken
Copy link
Contributor

philipreinken commented Mar 30, 2021

Hi @JoshuaBehrens , as Niklas said, we'd like to integrate this change, do you think you'll find the time to complete this?

@JoshuaBehrens
Copy link
Contributor Author

Hi @philipgatzka Not before Saturday. Should this make it into the 6.4?

@philipreinken
Copy link
Contributor

Glad to hear that, merging this before 6.4 is not a requirement and too short of a timeframe either way I think. I didn't intend to build up pressure here, just wanted to know whether/how we can proceed 🙂 As far as I can tell there's one suggestion still open, just let us know if you need any information or assistance with this or any other roadblocks.

@dfsoeten
Copy link

dfsoeten commented Jun 21, 2021

I see this pull request is labeled as "scheduled", when can I expect this PR to merged and make it into a future Shopware update? I'm also in need of this feature! 😎

@philipreinken
Copy link
Contributor

philipreinken commented Jun 21, 2021

Hi @dfsoeten , as mentioned above, this PR is still incomplete (I forgot to set the label correctly the last time, sorry for that 🙈). We intend to include the change as soon as it's finished.

@dfsoeten
Copy link

dfsoeten commented Jun 21, 2021

Ah, that's a mistake on my part then. Haven't read the entire conversation carefully enough. Thank you for clarifying
@philipgatzka !

@unugiyen
Copy link

unugiyen commented Jul 5, 2021

SO adding the last suggestion from @keulinho, and PR will be completed? @JoshuaBehrens @shyim

AFAIK:

Change this



if (!empty($input->getOption('locale'))) {
            $this->getApplication()->find('system:locale-destructive')->activateCommand();
            $commands[] = [
                'command' => 'system:locale-destructive',
                'locale' => $input->getOption('locale'),
            ];

            $commands[] = [
                'command' => 'cache:clear',
            ];
        }

To:

if (!empty($input->getOption('locale'))) {
            $this->getApplication()->find('system:locale-destructive')->activateCommand();
            $commands[] = [
                'command' => 'system:locale-destructive',
                'locale' => $input->getOption('locale'),
            ];
        }

ANd than use the cache clearer, since the cache clearer is is clearing in the SystemLocaleCommand?

        $this->setDefaultLanguage($locale);
        $this->cacheClearer->clear();

@JoshuaBehrens
Copy link
Contributor Author

@unugiyen I'll have a look at that :) thank you very much

@lukewenkers
Copy link

Hi @JoshuaBehrens,
are you still working on this issue or are you currently not working on it?

@JoshuaBehrens
Copy link
Contributor Author

I had a look at @unugiyen changes and they were good but I didn't integrate them yet. Shall I? :)

@lukewenkers
Copy link

Hi @JoshuaBehrens,
that would be great. 💙

@inventory69
Copy link

inventory69 commented Sep 14, 2021

Please i need this commit in the original shopware production template git <3 @JoshuaBehrens

@keulinho
Copy link
Contributor

We will provide an option to do this "shortly" ™️ in the shopware platform,
i'm not sure if switching the default language and currencies afterwards will be supported directly, but we will definetly add support to change the default language and currency during the installation process, which is the most pressing concern i guess.

@AndreasA
Copy link
Contributor

AndreasA commented Nov 3, 2021

@keulinho I think it is now, so I guess this PR could be closed?

After all with 6.4.6.0, there is a command to change it:

https://github.com/shopware/platform/blob/v6.4.6.0/src/Core/Maintenance/System/Command/SystemConfigureShopCommand.php

and it can also be set at the beginning:
https://github.com/shopware/platform/blob/v6.4.6.0/src/Core/Maintenance/System/Command/SystemInstallCommand.php#L41

@keulinho
Copy link
Contributor

keulinho commented Nov 4, 2021

@AndreasA Yep v6.4.6.0 already includes a command to change the language afterward, and you can set it during CLI set up, so i'm closing this PR.

If you have any issues or anything is missing in those commands, please open another issue or another PR.

@keulinho keulinho closed this Nov 4, 2021
@JoshuaBehrens
Copy link
Contributor Author

WE HAVE A COMMAND NOW :O 💙 💙 💙 💙 💙 💙 💙 💙

@JoshuaBehrens
Copy link
Contributor Author

I had a look at it and yes, I will amend it :) nothing troublesome so far. We will see how well it does in our next projects. Thank you so much @keulinho for this command!

@JoshuaBehrens JoshuaBehrens deleted the feature/command-to-set-system-locale branch November 4, 2021 08:34
@JoshuaBehrens
Copy link
Contributor Author

For all that are running into this. The command is not perfect yet! Change your default language ASAP after installation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.