Update flag_data column size

[cherednichenkoa]

data_flag column mostly used for config:import & config:dump console commands , one of the features is to save app:config:import data as a system_config_snapshot, if store has several scopes + multiple extensions, it can be not enough to save config snapshot

Description

This fix is related to config dump\import mechanism. If we run php bin/magento app:config:dump command, it saves snapshot of previous data to flag table with system_config_snapshot key. by default it is defined 64k.
I have faced this issue on 2.2.3 EE , I have 6 websites , and few custom extensions (not so much), however if I do app:config:import it generates config with size around 95 Kb, so serialized data in this column will be cropped (it will save everything it can save from serialized field , 64 kb and that's all, other 20 kb of serialized string in my case will be lose ).

As I have just few custom system configs, I am assuming this issue will appear on every site that has more that 3-4 website
flag table has very small amount of entries so we can little increase size of this column without any memory issue (but we will be sure issue does not appear again)

Manual testing scenarios

1. Create 6-7 websites
2. Run php bin/magento app:config:dump
3. Change something in config files, to be able to run php bin/magento app:config:import
4. Run php bin/magento app:config:import (system_config_snapshot should be created around here )
5. Repeat 3-4 and you will see "Import failed: Unable to unserialize value, string is corrupted." - if your system config size is more than 64kb, saved serialized string that exceeded 64 kb will be broken

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)

[miguelbalparda]

Hi @cherednichenkoa! This PR is really helpful, can you please review this and reformat accordingly?

[cherednichenkoa]

Hello @miguelbalparda , not sure that I got your point ? Do you want me to little refactor styling of the code, is it correct ?

[cherednichenkoa]

I see many white spaces that can be removed, however schema install body mostly moved to the separate methods and looks okay

[miguelbalparda]

Sorry if I wasn't clear, this wasn't about code styling. We need to update the DB field and not modify its value.
Like

        if (version_compare($context->getVersion(), '2.0.8', '<')) {
            $this->expandRemoteIpField($setup);

[cherednichenkoa]

Hello @miguelbalparda ,
I have already checked it before create PR, the problem is - this installer located in magento core installation file
setup/src/Magento/Setup/Model/Installer.php - this file does not have versioning - it executes only one time during magento installation, so it does not make a sense to split this method into table install + column modifying code .
Before PR i have checked all modules that depends on this table, and it looks like I can use Magento_Deploy, to set up new version and update 'flag' table.
However Magento_Deploy does not have schema at all , and I have to built entire new set of declarative schema in db_schema.xml for 'flag' table , but deploy module is not responsible for that .
Please let me know what you think about it ?

Also, not sure that I got your point - We need to update the DB field and not modify its value. - I have updated column size, not the value

[miguelbalparda]

You are right about the installer file, my bad.
What you modified is the value of the column size, so my original comment remains valid. What I wanted to explain you is that if you just modify the installer, this won't run in any installed version but only in the new installs, that's why we need to update the field.
@dmanners do you know what's the best approach to follow here?

[cherednichenkoa]

@miguelbalparda see my comment above regarding this concern,
if we want to move it into upgrade script, we need to declare entire new db schema in db_schema.xml
in magento-vendor module that we want to use for further maintain of this table
As this table defined in magento setup script, it can be related to magento installer, so such change can be really critical, I can move it to module, but before, I want to see confirmation regarding such change .
I will appreciate any suggestion

[miguelbalparda]

@dmanners can you help here?

[aschrammel]

We had the same problem already in #13579 (and #13580 as a ForwardPort).
Seems to me like the problem is not only ours and we abandoned the PR due to lack of information/help how we could manage this change in an updatescript.

[cherednichenkoa]

@aschrammel as we noticed above, it is impossible to change via update script especially in 2.2.3 with declarative schema

[miguelbalparda]

Closing as inactive, feel free to reopen if anything changes in the future.

[orlangur]

This is still not fixed in mainline and still relevant. This has nothing to do with schema upgrades as it is a part of installer.

Maybe it makes sense to increase column even more, to 1M or something?

[m2-assistant]

Hi @cherednichenkoa. 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 give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

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

[magento-engcom-team]

Hi @orlangur, thank you for the review.
ENGCOM-5522 has been created to process this Pull Request
✳️ @orlangur, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[orlangur]

Closing in favor of #13580, 128k could be too few.

[m2-assistant]

Hi @cherednichenkoa, 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.