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

Fix deprecated CHARSET utf8 in install and upgrade #144

Merged
merged 8 commits into from
Sep 7, 2023

Conversation

leemyongpakvn
Copy link
Contributor

@leemyongpakvn leemyongpakvn commented Jun 27, 2022

Questions Answers
Description? install script prior 5.0.1 6.0.0 version CREATE TABLE with deprecated CHARSET=utf8. It need to be updated to utf8mb4 in install and upgrade process of the next version.
Type? improvement
BC breaks? no
Deprecations? yes
Fixed ticket? Fixes PrestaShop/PrestaShop#28857
How to test? Check the above ticket.

install.sql in root directory of module prior v5.0.1 CREATE TABLE with deprecated CHARSET=utf8. It need to be fixed in the upgrade of next version.
@leemyongpakvn leemyongpakvn changed the title Replace deprecated CHARSET utf8 in install.sql by utf8mb4 Fix deprecated CHARSET utf8 in install and upgrade Jun 28, 2022
@kpodemski kpodemski added this to the 5.0.3 milestone Sep 26, 2022
@kpodemski
Copy link
Contributor

Hey @leemyongpakvn 👋

PHP-CS-Fixer is probably not happy with an indentation you have in your upgrade file

@leemyongpakvn
Copy link
Contributor Author

@kpodemski PHP-CS-Fixer v2 complains about 1 redundant space. I have no problem with PHP-CS-Fixer v3 on my localhost 😋

@florine2623 florine2623 self-assigned this Sep 26, 2022
Copy link
Contributor

@florine2623 florine2623 left a comment

Choose a reason for hiding this comment

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

Hello @leemyongpakvn ,

I have an issue, I have installed version 5.0.0 of productcomments module.
But I don't have the upgrade to v5.0.3 available, only v5.0.2:

Screenshot 2022-09-28 at 09 47 54

Could you check ?
Thanks!

@kpodemski
Copy link
Contributor

@florine2623 I think it's because you need to bump the version of the module in productcomments.php 🤔

@ghost
Copy link

ghost commented Sep 28, 2022

@florine2623

You must create version 5.0.3. Say me if you need help

@ghost
Copy link

ghost commented Sep 28, 2022

Ha ! @kpodemski again too fast

@florine2623
Copy link
Contributor

@kpodemski @okom3pom I though it would be automatic !

Can one of you guy show me how to do it ?

I changed it in productcomments.php line 48 $this->version = '5.0.3';. Not sure if that's the right way to do it 😅

@ghost
Copy link

ghost commented Sep 28, 2022

Here you have two things to test:

The update and the installation.

You have to check that the tables will be in utf8mb4

Yes, you have to change the version at this line

@florine2623
Copy link
Contributor

Here's what I did:

  • git clone [email protected]:PrestaShop/productcomments.git
  • click Install on BO
  • git checkout tags/v5.0.0
  • Install PR: git prc origin 144 dev
  • Modified productcomments.php line 48 $this->version = '5.0.3';
  • Checked BO, upgrade available only to 5.0.2. Not 5.0.3.

Am I missing something ?

@leemyongpakvn
Copy link
Contributor Author

@kpodemski & @okom3pom According to modules/creation/enabling-auto-update, the upgrade filename must be upgrade-5.0.3.php. Upgraded successfully on PS 1.7.8.5 but failed on PS 8.0.0 with filename upgrade-5.0.3.php.
Amazingly other previous upgrade files of this modules are install-version.php 🤔

@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Sep 28, 2022

@florine2623 Please change filename from install-5.0.3.php to upgrade-5.0.3.php in module's upgrade folder also, then try to upgrade again. Change filename from install to upgrade really does not help to show upgrade to correct version 5.0.3 :(

@leemyongpakvn
Copy link
Contributor Author

I have other more strange problem on PS 8.0.0. After clone module with git clone https://github.com/PrestaShop/productcomments.git, then install from BO, I still see 5.0.1 in BO although in productcomments.php line 48 $this->version = '5.0.2'
Change productcomments.php line 48 to $this->version = '5.0.3'; will lead to this strange screen
PS8 0_productcomments_5 0 2
It looks like the old version string 5.0.1 (in my case) is kept somewhere out of productcomments.php

@kpodemski
Copy link
Contributor

I'll ping @atomiix as it seems that it might be related to the Distribution API Client

@kpodemski
Copy link
Contributor

QA, the workaround is to disable ps_distributionapi before upgrading the module.

Ensure that module tables are all with a new encoding after upgrading to 5.0.3. Remember about changing 5.0.2 to 5.0.3 in productcomments.php

@kpodemski Nearly perfect. 👍 When run directly in phpMyAdmin: my code only upgrade table collation, your code upgrade both table and its fields collation.
The remaining issue is Upgrade via BO interface does not call install-**5.0.3**.php. BO upgrade suggest upgrade v5.0.2 -> v5.0.2, and I guess it called  install-**5.0.2**.php instead :(

Co-authored-by: Krystian Podemski <[email protected]>
@leemyongpakvn
Copy link
Contributor Author

@kpodemski Although product_comment_usefulness and product_comment_report tables don't have char/text fields, I think they still need CONVERT TO like others.
Another issue is COLLATION keywords at the end of every CREATE TABLE commands in install-dev. I don't know why they are there but I guess we need do the same for this module install.php.
Be free to add suggestions if you think they are needed :)

@Hlavtox Hlavtox modified the milestones: 5.0.3, 5.0.4 Mar 6, 2023
@Hlavtox Hlavtox modified the milestones: 5.0.4, 6.0.0 Jul 20, 2023
@leemyongpakvn leemyongpakvn modified the milestones: 6.0.0, 6.0.1 Aug 24, 2023
@davidglezz
Copy link

davidglezz commented Aug 24, 2023

I think there is a corner case:

User have old version installed and then uninstalls it keeping the tables.
User install new version.

I think that migration script is not executed so, that user will still having utf8 instead of utf8mb4

I think that we should check if tables exists on install.

Wow, this is a bigger problem, if the user decides to keep the old version tables, the columns might be different in a new version.

@leemyongpakvn
Copy link
Contributor Author

@davidglezz I see parameter $keep = true in productcomments.php's function uninstall,
but in Back-Office it looks like keep the module's files

Optional: delete module folder after uninstall.

not keep the module's tables.
If a user knows how to delete the module's files manually while keep its tables, then reinstall module, he/she must be a dev or a QA 😨

@leemyongpakvn
Copy link
Contributor Author

leemyongpakvn commented Aug 24, 2023

Fortunately, this install-6.0.1.php still runs when testing in a dev style (remove module folder, git clone, apply this PR changes, change module version to 6.0.1 in productcomments.php and config.xml, then upgrade via BO). A bit funny when I checked Module Upgrade in BO
productcomments_600_to_601

but Upgrade button did its task correctly: module tables COLATTION changed to utf8mb4, and module version become 6.0.1.

@leemyongpakvn leemyongpakvn modified the milestones: 6.0.1, 6.0.3 Aug 28, 2023
@nicosomb
Copy link
Contributor

nicosomb commented Sep 7, 2023

I installed PrestaShop develop locally.

Here is the ps_product_comment structure:

Capture d’écran 2023-09-07 à 11 10 42

I deleted the productcomments module, I installed the @leemyongpakvn productcomments module, and here is the table structure:

Capture d’écran 2023-09-07 à 11 10 46

utf8mb4_general_ci 🎉

I checked the other tables, it's OK too.

@leemyongpakvn
Copy link
Contributor Author

Thanks @kpodemski and @nicosomb

@leemyongpakvn leemyongpakvn merged commit ccb8fb1 into PrestaShop:dev Sep 7, 2023
8 checks passed
@leemyongpakvn
Copy link
Contributor Author

It is a bit surprised that install-6.0.3.php still works with function upgrade_module_6_0_1 inside ;)

@leemyongpakvn leemyongpakvn deleted the patch-3 branch January 4, 2024 14:34
@florine2623 florine2623 mentioned this pull request Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Fix deprecated CHARSET utf8 in native modules' install and upgrade