autoupgrade icon indicating copy to clipboard operation
autoupgrade copied to clipboard

Fix version compare check to use CoreUpgrader 8 with 8.0 version

Open FabienPapet opened this issue 2 years ago • 3 comments

This PR fixes CoreUpgrader file

If you want to update to version 8.0, the core upgrader uses the CoreUpgrader17 instead of CoreUpgrader8.

(the selected core upgrader depends on the targeted version and not from the source version)

Fixes https://github.com/PrestaShop/PrestaShop/issues/29248

HOW TO TEST:

1/ In the autoupgrade form, choose upgrade via local archive 2/ In the field for the upgrade version, you should only be able to enter versions with a patch precision, example:

  • 1.7.7.8
  • 8.0.0
  • 1.6.1.24

Examples of wrong versions (you will get an error message):

  • 1.7.2
  • 8.0
  • 8.0.0.0

(Notice how for PS 8 and above, the version is made of 3 numbers, while it's 4 numbers for older versions)

FabienPapet avatar Aug 16 '22 15:08 FabienPapet

Some weird PHP I tested to find the bug and help you understand this PR.

php > var_dump(version_compare("8.0", "8.0.0", '<'));
bool(true)
php > var_dump(version_compare("8.0.0", "8.0.0", '<'));
bool(false)
php > var_dump(version_compare("8", "8.0.0", '<'));
bool(true)

FabienPapet avatar Aug 16 '22 15:08 FabienPapet

@FabienPapet I added a commit with some wording modification

About the regex: did you test it in the form ? I tried your PR and it seems like I can put any value I want in the field, I don't think it comes from your regex, more like it's not even taken into account, but maybe I'm missing something ? 🤔

the regex seems good otherwise 👍

matthieu-rolland avatar Aug 19 '22 07:08 matthieu-rolland

@FabienPapet small reminder so the PR can be ready for the auto-upgrade release :)

MatShir avatar Aug 30 '22 15:08 MatShir

Should be good now

FabienPapet avatar Sep 05 '22 13:09 FabienPapet

Hello @khouloudbelguith I updated the pull request, now you will have this message if you don't give a correct version number

Capture d’écran 2022-09-15 à 11 12 15

matthieu-rolland avatar Sep 15 '22 09:09 matthieu-rolland

thank you @khouloudbelguith

I fixed your first issue :+1:

About the second one:

  • I can't reproduce it following your steps, I guess something else went wrong in your case but I'm not sure what, we should definitely investigate
  • I don't think it's linked to this PR (this PR only modifies JS, and version check in PHP, nothing else, it would be weird that it breaks the upgrade process... and I didn't reproduce with the PR)

Do you think we can create an issue for your second point, with reproducible steps, then it would be fixed in another PR

matthieu-rolland avatar Sep 20 '22 13:09 matthieu-rolland

Hello @FabienPapet @matthieu-rolland @matks !

As seen with @khouloudbelguith , since the second issue cannot be reproduced by everyone and It seems that it's more subtle than we thought (yeah, 17 steps where not enough).

So we will create an issue for this second issue and investigate on it later. So we only need the first issue to be fixed and we will test it to check the fix, if it's OK, then merge.

Robin-Fischer-PS avatar Sep 21 '22 09:09 Robin-Fischer-PS

Thank you everybody

matks avatar Sep 22 '22 19:09 matks