Add support for char element to dto factory #22442

wardcapp · 2019-04-21T08:09:22Z

To avoid the notice "Types char is not declared" (which breaks setup:upgrade) when using the very type anywhere in the MySQL database, mapping for this specific type is added to the DI as well.

Description

Adds the CHAR MySQL column type to the schema type declaration factory (di.xml). Which becomes required as soon as CHAR type has been used anywhere within the connected database - applies to non-Magento tables as well, since they are validated / scanned too upon executing setup:upgrade.

Fixed Issues

Was not able to find a report within the official GitHub repo on this, but still related is following community thread:
https://community.magento.com/t5/Magento-2-x-Version-Upgrades/Upgrade-Issue-2-2-7-to-2-3-0-Types-char-is-not-declared/td-p/114939

Manual testing scenarios

db_schema.xml

<?xml version="1.0"?>
<!--
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
-->
<schema xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="urn:magento:framework:Setup/Declaration/Schema/etc/schema.xsd">
    <table name="test_table" resource="default" engine="innodb" comment="Data">
        <column xsi:type="bigint" name="id" padding="20" unsigned="true" nullable="false" identity="true" comment="ID"/>
        <column xsi:type="char" name="dataChar" length="5" nullable="false" comment="data"/>
        <constraint xsi:type="primary" referenceId="PRIMARY">
            <column name="id"/>
        </constraint>
    </table>
</schema>

Structure of created table

mysql> show create table test_table\G
*************************** 1. row ***************************
       Table: test_table
Create Table: CREATE TABLE `test_table` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT COMMENT 'ID',
  `dataChar` char(5) NOT NULL COMMENT 'data',
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT='Data'
1 row in set (0.00 sec)

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

To avoid the notice "Types char is not declared" (which breaks setup:upgrade) when using the very type anywhere in the MySQL database, mapping for this specific type is added to the DI as well.

m2-assistant · 2019-04-21T08:09:24Z

Hi @wardcapp. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento-engcom-team give me test instance - deploy test instance based on PR changes
@magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

magento-cicd2 · 2019-04-21T08:09:25Z

All committers have signed the CLA.

orlangur

I believe this is slightly related to #1392. Why do we need to add CHAR mapping if it's not used in Magento?

wardcapp · 2019-04-22T08:10:24Z

This PR does slightly relate to #1392, which indeed explains why most of the users aren't applying the CHAR type throughout their MySQL DB over Magento setup scripts. Reasons to why CHAR should still be added to the DTO mapping / why the PR was created:

Although Magento core MySQL tables are currently not mentioning the CHAR column type, allowing others to do so (within the Magento DB, using custom setup script, Enqueue integration, other reasons to define custom tables ...) seems only reasonable.
Not having the mapping added to the ElementFactory and using CHAR anywhere in the DB breaks the setup:upgrade command, entirely (none of the module upgrades will be executed from then on).
Traces within the Magento core elsewhere are giving the impression CHAR was intended to be supported at first, but never completed - some entry-points:

Could be wrong still / assumptions made could be incorrect, so please feel free to provide feedback.

orlangur · 2019-04-22T09:11:07Z

Thanks @wardcapp!

@skovalenk @rganin @akaplya is there any reason to disallow CHAR column type?

Seb33300 · 2019-05-01T17:10:59Z

I am encountering the issue Types char is not declared when installing Magento on a shared database with other applications. Some of them are using the char field type and this result in this error...

This pull request could solve my issue.
But I don't really understand why db tables not related to Magento can break Magento install...

sidolov · 2019-07-15T20:17:21Z

app/etc/di.xml

@@ -1435,6 +1435,7 @@
                <item name="mediumtext" xsi:type="object">\Magento\Framework\Setup\Declaration\Schema\Dto\Factories\MediumText</item>
                <item name="text" xsi:type="object">\Magento\Framework\Setup\Declaration\Schema\Dto\Factories\Text</item>
                <item name="varchar" xsi:type="object">\Magento\Framework\Setup\Declaration\Schema\Dto\Factories\StringBinary</item>
+                <item name="char" xsi:type="object">\Magento\Framework\Setup\Declaration\Schema\Dto\Factories\StringBinary</item>


It's not enough to add char in di.xml, you need to fix db_scrma definition too

First of all, thank you for the follow-up! Not quite sure if I am understanding this correctly - did you mean the DB schema type declarations (within the very same di.xml)? If so, char has already been defined there as well within the current 2.3-develop branch (see app/etc/di.xml:1471).

Apart from that, it is possible to add support for the char type to Magento 2 setup scripts. Which would require adding of a char schema setup declaration (similar to lib/internal/Magento/Framework/Setup/Declaration/Schema/etc/schema.xsd:22) indeed, but does not relate to the intent of this pull request (and could be set up over a separate PR or feature request). But I might still be overlooking another required type of definition?

@sidolov please respond :)

Hi @wardcapp This is a good idea extending compatible types, but unfortunately, this is not as easy as change one XSD. Here is an example of adding a new data type to a declarative schema. https://github.com/magento/magento2/pull/25479/files
In your case, it even could be more complicated because the CHAR type supports more options that JSON. Also, you can take a look at VARCHAR implementation as an example.

engcom-Golf · 2020-02-07T21:29:12Z

i will provide support for db_schema

…ctory

lenaorobei · 2020-02-07T21:32:50Z

@wardcapp could you please sign CLA so our team will be able to finish this PR?

wardcapp · 2020-02-07T21:48:53Z

Hi @lenaorobei, CLA has been signed and related check should pass now.

orlangur

@Nazar65 looks good to me except the small typo 👍

Please fix it and squash your changes into a single commit.

orlangur · 2020-02-14T17:34:09Z

lib/internal/Magento/Framework/Setup/Declaration/Schema/etc/types/texts/char.xsd

+            <xs:extension base="abstractColumnType">
+                <xs:annotation>
+                    <xs:documentation>
+                        Here plain text can be persisted without trailing spaces. Length of this field cant be more than 255 characters


Typo: "cannot" or "can't"

ok, thank you 👍

engcom-Golf · 2020-02-17T09:27:12Z

@orlangur thank you for your review, done ✔️

engcom-Alfa · 2020-02-27T13:01:42Z

✔️ QA Passed

engcom-Golf · 2020-02-27T15:22:08Z

@magento run all tests

engcom-Golf · 2020-02-27T15:23:22Z

@magento run all tests

magento-engcom-team · 2020-03-02T18:41:40Z

Hi @lenaorobei, thank you for the review.
ENGCOM-7016 has been created to process this Pull Request

m2-assistant · 2020-03-12T04:28:30Z

Hi @wardcapp, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Add support for char element to dto factory

49ff38a

To avoid the notice "Types char is not declared" (which breaks setup:upgrade) when using the very type anywhere in the MySQL database, mapping for this specific type is added to the DI as well.

magento-engcom-team added the Release Line: 2.3 label Apr 21, 2019

orlangur self-assigned this Apr 21, 2019

orlangur requested changes Apr 21, 2019

View reviewed changes

ghost added the Progress: needs update label Apr 21, 2019

Hanhan1989 approved these changes May 15, 2019

View reviewed changes

sidolov requested changes Jul 15, 2019

View reviewed changes

ghost assigned sidolov Jul 15, 2019

sidolov changed the base branch from 2.3-develop to 2.4-develop December 5, 2019 17:21

sidolov added Release Line: 2.4 and removed Release Line: 2.3 labels Dec 5, 2019

lenaorobei self-assigned this Feb 6, 2020

engcom-Golf self-assigned this Feb 7, 2020

Merge branch '2.4-develop' into magento-2.3/add-char-to-dto-elementfa…

c876e0d

…ctory

engcom-Golf force-pushed the magento-2.3/add-char-to-dto-elementfactory branch 3 times, most recently from 7c5471c to 8741cd0 Compare February 11, 2020 12:54

Nazar65 requested a review from akaplya February 11, 2020 17:39

orlangur requested changes Feb 14, 2020

View reviewed changes

engcom-Golf force-pushed the magento-2.3/add-char-to-dto-elementfactory branch from c23a0a5 to 58dde94 Compare February 17, 2020 09:25

Add support for db_schema.xml char type column

58dde94

ghost added Progress: ready for testing and removed Progress: needs update labels Feb 26, 2020

lenaorobei added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Covered All changes in Pull Request is covered by auto-tests labels Feb 26, 2020

engcom-Alfa self-assigned this Feb 27, 2020

ghost added Progress: testing in progress and removed Progress: ready for testing labels Feb 27, 2020

engcom-Golf added the Auto-Tests: Covered All changes in Pull Request is covered by auto-tests label Feb 27, 2020

cover changes with setup-integration test

b1d4c72

ghost added Progress: extended testing and removed Progress: testing in progress labels Feb 27, 2020

add cgar column to fixtures

6070770

added assertion

92ec594

ghost added Progress: accept and removed Progress: extended testing labels Feb 27, 2020

lenaorobei approved these changes Mar 2, 2020

View reviewed changes

ghost added Progress: ready for testing Progress: testing in progress and removed Progress: accept Progress: ready for testing labels Mar 2, 2020

magento-engcom-team added the Component: Setup label Mar 2, 2020

ghost added Progress: accept and removed Progress: testing in progress labels Mar 2, 2020

magento-engcom-team pushed a commit that referenced this pull request Mar 12, 2020

ENGCOM-7016: Add support for char element to dto factory #22442

5e616b2

magento-engcom-team merged commit b82791d into magento:2.4-develop Mar 12, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for char element to dto factory #22442

Add support for char element to dto factory #22442

wardcapp commented Apr 21, 2019 •

edited by engcom-Golf

Loading

m2-assistant bot commented Apr 21, 2019

magento-cicd2 commented Apr 21, 2019 •

edited

Loading

orlangur left a comment

wardcapp commented Apr 22, 2019 •

edited

Loading

orlangur commented Apr 22, 2019

Seb33300 commented May 1, 2019 •

edited

Loading

sidolov Jul 15, 2019

wardcapp Jul 21, 2019 •

edited

Loading

orlangur Sep 22, 2019

akaplya Feb 6, 2020

engcom-Golf commented Feb 7, 2020

lenaorobei commented Feb 7, 2020

wardcapp commented Feb 7, 2020

orlangur left a comment

orlangur Feb 14, 2020

engcom-Golf Feb 17, 2020

engcom-Golf commented Feb 17, 2020 •

edited

Loading

engcom-Alfa commented Feb 27, 2020

engcom-Golf commented Feb 27, 2020

engcom-Golf commented Feb 27, 2020

magento-engcom-team commented Mar 2, 2020

m2-assistant bot commented Mar 12, 2020

Add support for char element to dto factory #22442

Add support for char element to dto factory #22442

Conversation

wardcapp commented Apr 21, 2019 • edited by engcom-Golf Loading

Description

Fixed Issues

Manual testing scenarios

db_schema.xml

Structure of created table

Contribution checklist

m2-assistant bot commented Apr 21, 2019

magento-cicd2 commented Apr 21, 2019 • edited Loading

orlangur left a comment

Choose a reason for hiding this comment

wardcapp commented Apr 22, 2019 • edited Loading

orlangur commented Apr 22, 2019

Seb33300 commented May 1, 2019 • edited Loading

sidolov Jul 15, 2019

Choose a reason for hiding this comment

wardcapp Jul 21, 2019 • edited Loading

Choose a reason for hiding this comment

orlangur Sep 22, 2019

Choose a reason for hiding this comment

akaplya Feb 6, 2020

Choose a reason for hiding this comment

engcom-Golf commented Feb 7, 2020

lenaorobei commented Feb 7, 2020

wardcapp commented Feb 7, 2020

orlangur left a comment

Choose a reason for hiding this comment

orlangur Feb 14, 2020

Choose a reason for hiding this comment

engcom-Golf Feb 17, 2020

Choose a reason for hiding this comment

engcom-Golf commented Feb 17, 2020 • edited Loading

engcom-Alfa commented Feb 27, 2020

engcom-Golf commented Feb 27, 2020

engcom-Golf commented Feb 27, 2020

magento-engcom-team commented Mar 2, 2020

m2-assistant bot commented Mar 12, 2020

wardcapp commented Apr 21, 2019 •

edited by engcom-Golf

Loading

magento-cicd2 commented Apr 21, 2019 •

edited

Loading

wardcapp commented Apr 22, 2019 •

edited

Loading

Seb33300 commented May 1, 2019 •

edited

Loading

wardcapp Jul 21, 2019 •

edited

Loading

engcom-Golf commented Feb 17, 2020 •

edited

Loading