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

Unify calls to list directory contents #695

Merged

Conversation

Quetzacoalt91
Copy link
Member

@Quetzacoalt91 Quetzacoalt91 commented Apr 26, 2024

Questions Answers
Description? They are several ways to list the content of a directory, and some issues were met. This PR adds tests and plugs all the calls to a same method.
Type? refacto
BC breaks? Nope
Deprecations? Nope
Fixed ticket? Need to find it
Sponsor company PrestaShopCorp
How to test? Copy of files during upgrades, backups and restores work the same as before.

To Do list

  • Ignore modules during upgrade Reverted
  • Remove duplicated method listing files in UpgradeFiles.php
  • Test generation of files list for a rollback
  • Test generation of files list for a backup

Known issues

  • These issues will be reported because the module is not copied anymore during upgrade
[INTERNAL] /var/www/html/modules/autoupgrade/vendor/composer/ClassLoader.php line 571 - include(/var/www/html/vendor/composer/../../modules/ps_distributionapiclient/ps_distributionapiclient.php): failed to open stream: No such file or directory
[INTERNAL] /var/www/html/modules/autoupgrade/vendor/composer/ClassLoader.php line 571 - include(): Failed opening '/var/www/html/vendor/composer/../../modules/ps_distributionapiclient/ps_distributionapiclient.php' for inclusion (include_path='/var/www/html/vendor/pear/pear_exception:/var/www/html/vendor/pear/console_getopt:/var/www/html/vendor/pear/pear-core-minimal/src:/var/www/html/vendor/pear/archive_tar:.:/usr/local/lib/php')

This will be fixed when the upgrade of modules is improved. In the meantime ignoring the whole modules/ folder has been reverted.

Issue found with the current version

Capture d’écran du 2024-04-19 14-22-27

@Quetzacoalt91 Quetzacoalt91 marked this pull request as draft April 26, 2024 15:52
@Quetzacoalt91 Quetzacoalt91 self-assigned this Apr 26, 2024
@Quetzacoalt91 Quetzacoalt91 force-pushed the rework-file-ignoring-system branch from 6076961 to 18267e0 Compare April 26, 2024 16:01
@Hlavtox
Copy link
Contributor

Hlavtox commented Apr 26, 2024

Guys please make sure we don't reintroduce some issues previously fixed, like - #651 🙏

@Quetzacoalt91
Copy link
Member Author

Quetzacoalt91 commented Apr 29, 2024

Guys please make sure we don't reintroduce some issues previously fixed, like - #651 🙏

The test I'm adding is supposed to cover the case you are talking about in your link, even if we're talking about modules instead of install.
The file of expected files to upgrade has a modules folder we can find deep in the tree. The root modules folder has been ignored, but this one remains.

"\/admin-dev\/themes\/default\/scss\/modules",
"\/admin-dev\/themes\/default\/scss\/modules\/_colors.scss",
"\/admin-dev\/themes\/default\/scss\/modules\/_mixins.scss",
"\/admin-dev\/themes\/default\/scss\/modules\/_variables.scss",
"\/admin-dev\/themes\/default\/scss\/modules\/index.php",

There is another case that could be covered though, where the case can be different (i.e install vs Installer). I'll have a look at it too.

@Quetzacoalt91 Quetzacoalt91 force-pushed the rework-file-ignoring-system branch from 05d7e94 to 367f0b1 Compare April 29, 2024 15:39
@Hlavtox
Copy link
Contributor

Hlavtox commented Apr 30, 2024

@Quetzacoalt91 Cool, good job!!! :-)

@Quetzacoalt91 Quetzacoalt91 force-pushed the rework-file-ignoring-system branch from 2935c36 to f5bd50f Compare May 1, 2024 16:01
@Hlavtox
Copy link
Contributor

Hlavtox commented May 1, 2024

One idea @Quetzacoalt91, do you think it would be to use this opportunity to fix one annoying behavior? The current logic copies all of the native modules in the zip to the install. Maybe we could copy them only if the folder already exists?

cc @MatShir and @kpodemski

@Quetzacoalt91 Quetzacoalt91 force-pushed the rework-file-ignoring-system branch from 04e1cc4 to b980555 Compare May 1, 2024 16:50
@Quetzacoalt91
Copy link
Member Author

One idea @Quetzacoalt91, do you think it would be to use this opportunity to fix one annoying behavior? The current logic copies all of the native modules in the zip to the install. Maybe we could copy them only if the folder already exists?

cc @MatShir and @kpodemski

There are a few improvement we'd like to add in the upgrade process, something a bit smarter when we deal about modules (i.e avoiding rollback while copying, check if a module is new in the new release vs a simple upgrade). This PR makes us ready to stop copying the modules folder once we have something ready to improve module management during an upgrade.

@Quetzacoalt91 Quetzacoalt91 marked this pull request as ready for review May 1, 2024 16:57
@Quetzacoalt91 Quetzacoalt91 force-pushed the rework-file-ignoring-system branch from b980555 to df2af67 Compare May 15, 2024 16:16
@Quetzacoalt91 Quetzacoalt91 force-pushed the rework-file-ignoring-system branch from df2af67 to b9f772d Compare May 20, 2024 15:55
@AureRita AureRita self-assigned this May 23, 2024
Copy link
Contributor

@AureRita AureRita left a comment

Choose a reason for hiding this comment

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

Hi @Quetzacoalt91

Thank you for your PR, I tested it and it seems to works as you can see :

recording.214.webm

Tested on :
8.0.5 to 8.1.6 from major release, I've done a rollback after this
8.1.5 to 8.1.6 from minor release, I've done a rollback after this
8.0.5 to 9.0.0 from local release
8.1.5 to 9.0.0 from local release
1.7.8.11 to 8.1.6 from major release

Because the PR seems to works as expected, It's QA ✔️

Thank you

@Quetzacoalt91 Quetzacoalt91 merged commit bdbbaab into PrestaShop:dev May 23, 2024
29 checks passed
@Quetzacoalt91 Quetzacoalt91 deleted the rework-file-ignoring-system branch May 23, 2024 14:14
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.

6 participants