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

Workaround for MariaDB >= 10.2.7 compatibility #1396

Conversation

samuelvogel
Copy link

@samuelvogel samuelvogel commented Nov 22, 2017

1. Why is this change necessary?

When Shopware is run with MariaDB and the old articles attributes definition from before 5.3 is still present (s_articles_attributes) illegal attribute models are generated based on the column definition:

`attr17` date DEFAULT NULL

MariaDB is not officially supported but nevertheless "recommended" in the Shopware performance guide and also by some agencies.

The broken code is generated in /var/cache/production____REVISION___/doctrine/attributes/Article.php:

    /**
     * @var date $attr17
     *
     * @ORM\Column(name="attr17", type="date", nullable=true)
     */
    protected $attr17;

...

    public function __construct()
    {
        ...
        $this->attr15 = "NULL";
        $this->attr16 = "NULL";
        $this->attr17 = new \DateTime("NULL");
        $this->attr18 = "NULL";
        ...
    }

The following exception is thrown, for example when duplicating an article in the backend:
DateTime::__construct(): Failed to parse time string (NULL) at position 0 (N): The timezone could not be found in the database in var/cache/production____REVISION___/doctrine/attributes/Article.php on line 299

The attribute (s_articles_attributes.attr17) was normalized with Shopware 5.3, but this only applies for fresh installations not existing ones which are updated: https://github.com/shopware/shopware/blob/5325f6c831e4f6101ae1669494323475a38dce58/_sql/migrations/808-change-varchar-attributes.php#L32
And it's still possible to create problematic date attributes via the backend:
screenshot 2017-11-22 15 14 48

But the exception thrown for DATE columns is not the only problem, all attributes without a default value will be initialized with the string "NULL" (see attr15 above for example) with all MariaDB versions >= 10.2.7. The MariaDB handling of default values inINFORMATION_SCHEMA deviates from MySQL and the MariaDB devs decided that they do not want to change that anymore, but Doctrine is working on compatibility to both approaches. All of this discussion can be found here (including the linked issues): doctrine/orm#6565

2. What does this change do, exactly?

Ignore "NULL" (string) default values, which probably no shop owner needs/wants or would intentionally set. This is a workaround but should be manageable.

Apparently doctrine/dbal 2.7.0 will resolve this as well, but even on the Shopware 5.4 branch version 2.5.4 is still shipped.

3. Describe each step to reproduce the issue or behaviour.

Use MariaDB => 10.2.7 (docker setup documented here) and run ant build-unit or generate the attribute models some other way. The will use "NULL" (string) as default value for all 20 article attributes that are shipped by default.

4. Please link to the relevant issues (if any).

  1. https://issues.shopware.com/issues/SW-20274
  2. https://issues.shopware.com/issues/SW-20343

5. Which documentation changes (if any) need to be made because of this PR?

MySQL 5.7 was released 2015-10-21 and made most of the reasons for recommending MariaDB (better performance and easier configuration/more sensible defaults) obsolete. So really MariaDB could be removed from the Shopware performance docs. It looks like in the future MariaDB will care even less about MySQL compatibility.

6. Checklist

  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I have read the contribution requirements and fulfil them.
    I've verified this with MariaDB 10.2.10 vs. MySQL 5.7.20 but I do not think there is a realistic way to write automated tests for this right now (either mocking a big part of dbal or adding additional travis builds for MariaDB, which is supported BTW)

Attribute default models are broken otherwise, see: doctrine/orm#6565
@shopwareBot
Copy link

Hello,

thank you for creating this pull request.
I have opened an issue on our Issue Tracker for you. See the issue link: https://issues.shopware.com/issues/SW-20584

Please use this issue to track the state of your pull request.

@soebbing
Copy link
Contributor

Hi @samuelvogel, I just merged your PR as shopware/shopware@8cc5963 - thank you so much for this fix! 💙

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.

3 participants