fix: customer.language maps to shopId, not localeId - adding correct association

[PantherX99]

when migrating, unnecessary / unused languages are created. this is due to the mapping of customer.language which maps to the shopid and not the localeid.

this causes SWAG_MIGRATION_WRITE_EXCEPTION errors, because it assigns a wrong language id to the customer, so the customer cant be created and their order neither as well.

1. [/languageId] The language "019e88ab8071730686a04a88ca695f99" is not assigned to the sales channel. in /var/www/vhosts/c8yb4.creoline.cloud/httpdocs/vendor/shopware/core/Framework/DataAbstractionLayer/Write/WriteContext.php:46
   Converted data (JSON):

   /**

   • Id of the language sub shop
   • var string
   • Orm\Column(name="language", type="string", length=10, nullable=false)
     */
     private $languageId = '1';

[Jozsef Damokos (jozsefdamokos)]

Can we have a look at the tests too?

CustomerReaderTest:
static::assertSame('de-DE', $data[2]['customerlanguage']['locale']);

this should be failing no?

Also LanguageReaderTest should be updated as well

[PantherX99]

Jozsef Damokos (@jozsefdamokos) i'm new to this, sorry for any mistakes. i hope it's allright now.

[PantherX99]

https://github.com/shopware/SwagMigrationConnector/pull/13
I added the same fix to the sw5 connector plugin

[Jozsef Damokos (jozsefdamokos)]

PantherX99 no worries, we appreciate you taking the time to create the PR 😉

What i meant is that if indeed the shop id was used instead of the language id the assertion in CustomerReaderTest should have failed, no? or was it passing only by chance because the ids were matching?

Let's not add that extra shop in the main database file, instead we can insert it if needed in the test file itself, like in LanguageReaderTest::setUp

And i still feel like LanguageReaderTest should also be updated so it checks your changes

[PantherX99]

What i meant is that if indeed the shop id was used instead of the language id the assertion in CustomerReaderTest should have failed, no? or was it passing only by chance because the ids were matching?

Jozsef Damokos (@jozsefdamokos) The default shops German and English have the ids 1 and 2 which actually match their locale ids for de_DE and en_GB. But as soon as you add another shop (id 3) with a different locale, it resolves to the locale with the id 3 which is Afar-Dschibuti and not the actual locale. So the ids were matching because it used the default config and no custom subshop was added in the test.

[PantherX99]

Jozsef Damokos (@jozsefdamokos) Sorry, then it needs further support from some of you.

This test will fail because it directly assigns the locale to the customer.language property. The reader won't find a shop with id 9999 and only returns the locales for which a shop exists. So 'ar_EG' wont be returned as expected from the test.
Question is, what is the expected behaviour here?

public function testReadIncludesCustomerLocalesOutsideShopLocales(): void
    {
        $existingLocaleId = $this->dbConnection->fetchOne(
            'SELECT id FROM s_core_locales WHERE locale = :locale',
            ['locale' => 'ar_EG']
        );

        if ($existingLocaleId === false) {
            $this->dbConnection->executeStatement(
                'INSERT INTO s_core_locales (id, locale, language, territory) VALUES (:id, :locale, :language, :territory)',
                [
                    'id' => $this->customerLocaleId,
                    'locale' => 'ar_EG',
                    'language' => 'Arabic',
                    'territory' => 'Egypt',
                ]
            );

            $this->customerLocaleInserted = true;
        } else {
            $this->customerLocaleId = (int) $existingLocaleId;
        }

        $this->dbConnection->executeStatement('UPDATE s_user SET language = :language WHERE id = :id', [
            'language' => $this->customerLocaleId,
            'id' => 1,
        ]);

        $data = $this->languageReader->read($this->migrationContext);

        static::assertCount(3, $data);
        static::assertContains('ar-EG', \array_column($data, 'locale'));
    }

As there might be customers in the with a language value of a deleted shop (i dont know if this is even possible in sw5 but since a test for this case exists, i assume it is), it wont find a locale and $data['customerlanguage']['locale'] will be null. The CustomerConverter wont set a locale either, so the languageId in the converted data will be missing. This should cause a write fail because the language_id is NOT NULL.
To avoid this, maybe the converter should fall back to the mainlocale if no locale via the shop mapping is found?

if (!isset($converted['languageId'])) {
    $languageUuid = $this->resolveLanguageId(
        $this->mainLocale,
        $this->languageLookup,
        $context,
    );

    if ($languageUuid !== null) {
        $converted['languageId'] = $languageUuid;
    }
}

To test if my changes work, you should create a shop in the setup function with a different locale, modify the users language to this shop id and then test if it properly retrieves the correct locale. In addition to that, a user with an old/deleted language (shop) should be created to test the above mentioned case. But therefore the expected behaviour has to be determined.
Is that what you meant?

[Jozsef Damokos (jozsefdamokos)]

Please revert this addition here.

Instead we should add this special scenario in CustomerReaderTest andLanguageReaderTest

// in setUp method:
$this->dbConnection->executeStatement(
    'INSERT INTO s_core_shops (
        id, name, position, hosts, secure, locale_id, customer_scope, `default`, active
    ) VALUES (
        999, "English shop", 0, "", 0, 2, 0, 0, 1
    )'
);

$this->dbConnection->executeStatement('UPDATE s_user SET language = :language WHERE id = :id', [
    'language' => 999,
    'id' => 1,
]);

in LanguageReaderTest we can remove testReadIncludesCustomerLocalesOutsideShopLocales so it becomes something like:

class LanguageReaderTest extends TestCase
{
    use LocalCredentialTrait;

    private LanguageReader $languageReader;

    private MigrationContext $migrationContext;

    private Connection $dbConnection;

    protected function setUp(): void
    {
        $this->connectionSetup();

        $connectionFactory = new ConnectionFactory();
        $this->languageReader = new LanguageReader($connectionFactory);

        $this->migrationContext = new MigrationContext(
            $this->connection,
            new Shopware55Profile(),
            null,
            new LanguageDataSet(),
            $this->runId,
            0,
            10
        );

        $this->migrationContext->setGateway(new DummyLocalGateway());

        $this->dbConnection = $connectionFactory->createDatabaseConnection($this->migrationContext);

        $this->dbConnection->executeStatement(
            'INSERT INTO s_core_shops (
                id, name, position, hosts, secure, locale_id, customer_scope, `default`, active
            ) VALUES (
                999, "English shop", 0, "", 0, 2, 0, 0, 1
            )'
        );
        $this->dbConnection->executeStatement('UPDATE s_user SET language = :language WHERE id = :id', [
            'language' => 999,
            'id' => 1,
        ]);
    }

    protected function tearDown(): void
    {
        $this->dbConnection->executeStatement('UPDATE s_user SET language = :language WHERE id = :id', [
            'language' => 1,
            'id' => 1,
        ]);
        $this->dbConnection->executeStatement('DELETE FROM s_core_shops WHERE id = :id', [
            'id' => 999,
        ]);
    }

    public function testRead(): void
    {
        static::assertTrue($this->languageReader->supports($this->migrationContext));

        $data = $this->languageReader->read($this->migrationContext);
        $locales = \array_column($data, 'locale');

        static::assertCount(2, $data);
        static::assertContains('de-DE', $locales);
        static::assertContains('en-GB', $locales);
        static::assertNotContains('bn-IN', $locales);
    }
}

[Jozsef Damokos (jozsefdamokos)]

Same here, please revert