Refactor Codebase to Use Symfony Filesystem Instead of Native PHP Functions #1109

M0rgan01 · 2025-01-09T13:02:47Z

Questions	Answers
Description?	see beelow
Type?	refactor
BC breaks?	no
Deprecations?	no
Fixed ticket?	-
Sponsor company	-
How to test?	the refactoring of this PR affects all file-related operations, such as directory creation, copying or file deletion. It is therefore necessary to ensure that the operations on the files during the various processes have not had any degradation.

Description

This pull request refactors the codebase to replace the usage of native PHP filesystem functions (e.g., file_exists, unlink, mkdir, etc.) with the Symfony Filesystem component. This change improves code readability, consistency, and reliability by leveraging Symfony's robust and well-tested API for filesystem operations.

Key changes include:

Replacing direct calls to native filesystem functions with equivalent methods from the Symfony Filesystem component.
Injecting the Symfony Filesystem service where applicable to ensure better testability and adherence to dependency injection principles.
Updating existing tests to account for the refactored implementation.

This refactor aligns with best practices and enhances compatibility with modern Symfony projects.

ga-devfront · 2025-01-10T14:09:28Z

classes/Task/Update/UpdateComplete.php

+
+        if ($filesystem->exists($filePath)) {
+            if ($this->container->getUpdateConfiguration()->isChannelOnline()) {
+                try {
+                    $filesystem->remove($filePath);
+                    $this->logger->debug($this->translator->trans('%s removed', [$filePath]));
+                } catch (Exception $e) {


I'm not sure but I think you can use IOExceptionInterface from symfony filesystem.

ga-devfront · 2025-01-10T14:09:52Z

classes/Task/Update/UpdateComplete.php

+            try {
+                $filesystem->remove($latestPath);
+                $this->logger->debug($this->translator->trans('%s removed', [$latestPath]));
+            } catch (Exception $e) {
+                $this->logger->debug($this->translator->trans('Please remove %s by FTP', [$latestPath]));
+            }


you do the same as other try catch what do you think about make a method for it ?

classes/Task/Update/UpdateComplete.php

ga-devfront · 2025-01-10T14:11:43Z

classes/Task/Update/UpdateFiles.php

                    $this->next = TaskName::TASK_ERROR;
-                    $this->logger->error($this->translator->trans('Error while creating directory %s.', [$dest]));
+                    $this->logger->error($this->translator->trans('Error while creating directory %s: %s.', [$dest, $e->getMessage()]));


I'm not sure but I think something is wrong with the translation %s: %s

It looks fine to me, each value of the array will replace an occurrence of %s

ga-devfront · 2025-01-10T14:12:09Z

classes/Task/Update/UpdateFiles.php

                $this->next = TaskName::TASK_ERROR;
-                $this->logger->error($this->translator->trans('Error while copying file %s', [$file]));
+                $this->logger->error($this->translator->trans('Error while copying file %s: %s', [$file, $e->getMessage()]));


same for translation her

ga-devfront · 2025-01-10T14:59:08Z

Quetzacoalt91 · 2025-01-10T16:02:59Z

classes/Workspace.php

+            if (!$this->filesystem->exists($path)) {
+                try {
+                    $this->filesystem->mkdir($path);
+                } catch (IOException $e) {
+                    throw new IOException($this->translator->trans('Unable to create directory %s: %s', [$path, $e->getMessage()]));
+                }


No need to check anymore if the directory already exists. If it does, Filesystem will ignore the creation.

sonarqubecloud · 2025-01-24T10:55:30Z

Quality Gate failed

Failed conditions
4.5% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

paulnoelcholot · 2025-01-28T10:10:15Z

Blocked by : https://forge.prestashop.com/browse/SUE-308

AureRita

Hi @M0rgan01

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

1109_1788_820.mp4

I didn't see any op cache issue on this PR, thank you for this coreection

Tested from :
1.7.8.8 to 8.2
8.0.4 to 8.2
8.2 to 9.0.0
8.0.4 to 9.0.0

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

Thank you

M0rgan01 force-pushed the Filesystem-refacto branch 7 times, most recently from d9b2502 to 0c9d13d Compare January 9, 2025 17:07

M0rgan01 added this to the 7.0.0 milestone Jan 9, 2025

M0rgan01 force-pushed the Filesystem-refacto branch 2 times, most recently from 210af31 to 52308ff Compare January 10, 2025 09:08

M0rgan01 added the Blocked Status: The issue is blocked by another task label Jan 10, 2025

M0rgan01 changed the title ~~[WIP] Symfony FileSystem refacto~~ Refactor Codebase to Use Symfony Filesystem Instead of Native PHP Functions Jan 10, 2025

M0rgan01 marked this pull request as ready for review January 10, 2025 09:17

M0rgan01 closed this Jan 10, 2025

M0rgan01 reopened this Jan 10, 2025

PrestaShop deleted a comment from sonarqubecloud bot Jan 10, 2025

M0rgan01 removed the Blocked Status: The issue is blocked by another task label Jan 10, 2025

ga-devfront reviewed Jan 10, 2025

View reviewed changes

M0rgan01 force-pushed the Filesystem-refacto branch 3 times, most recently from 8c6447e to 5af8958 Compare January 10, 2025 14:44

M0rgan01 force-pushed the Filesystem-refacto branch from 5af8958 to 0f6f416 Compare January 10, 2025 15:01

Quetzacoalt91 reviewed Jan 10, 2025

View reviewed changes

M0rgan01 force-pushed the Filesystem-refacto branch 2 times, most recently from 4486a1c to dc6d365 Compare January 13, 2025 09:25

Quetzacoalt91 previously approved these changes Jan 13, 2025

View reviewed changes

M0rgan01 added the waiting for QA label Jan 15, 2025

M0rgan01 dismissed Quetzacoalt91’s stale review via d035f70 January 17, 2025 09:21

M0rgan01 force-pushed the Filesystem-refacto branch from dc6d365 to d035f70 Compare January 17, 2025 09:21

M0rgan01 force-pushed the Filesystem-refacto branch 13 times, most recently from b92d741 to db4db76 Compare January 24, 2025 09:19

M0rgan01 marked this pull request as draft January 24, 2025 09:20

M0rgan01 force-pushed the Filesystem-refacto branch from e2bf9bf to 11e5397 Compare January 24, 2025 10:48

Fix copy function

7da2368

M0rgan01 force-pushed the Filesystem-refacto branch from 11e5397 to 7da2368 Compare January 24, 2025 10:54

M0rgan01 removed the wip label Jan 24, 2025

M0rgan01 marked this pull request as ready for review January 24, 2025 10:54

M0rgan01 closed this Jan 24, 2025

M0rgan01 reopened this Jan 24, 2025

paulnoelcholot self-assigned this Jan 27, 2025

ga-devfront approved these changes Jan 27, 2025

View reviewed changes

ps-jarvis added the waiting for QA label Jan 27, 2025

AureRita approved these changes Jan 29, 2025

View reviewed changes

AureRita added QA ✔️ and removed waiting for QA labels Jan 29, 2025

M0rgan01 merged commit 967666f into PrestaShop:dev Jan 29, 2025
42 of 61 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Codebase to Use Symfony Filesystem Instead of Native PHP Functions #1109

Refactor Codebase to Use Symfony Filesystem Instead of Native PHP Functions #1109

M0rgan01 commented Jan 9, 2025 •

edited

Loading

ga-devfront Jan 10, 2025

ga-devfront Jan 10, 2025

ga-devfront Jan 10, 2025

Quetzacoalt91 Jan 17, 2025

ga-devfront Jan 10, 2025

ga-devfront commented Jan 10, 2025

Quetzacoalt91 Jan 10, 2025

sonarqubecloud bot commented Jan 24, 2025

paulnoelcholot commented Jan 28, 2025

AureRita left a comment

Refactor Codebase to Use Symfony Filesystem Instead of Native PHP Functions #1109

Refactor Codebase to Use Symfony Filesystem Instead of Native PHP Functions #1109

Conversation

M0rgan01 commented Jan 9, 2025 • edited Loading

Description

ga-devfront Jan 10, 2025

Choose a reason for hiding this comment

ga-devfront Jan 10, 2025

Choose a reason for hiding this comment

ga-devfront Jan 10, 2025

Choose a reason for hiding this comment

Quetzacoalt91 Jan 17, 2025

Choose a reason for hiding this comment

ga-devfront Jan 10, 2025

Choose a reason for hiding this comment

ga-devfront commented Jan 10, 2025

Quetzacoalt91 Jan 10, 2025

Choose a reason for hiding this comment

sonarqubecloud bot commented Jan 24, 2025

Quality Gate failed

paulnoelcholot commented Jan 28, 2025

AureRita left a comment

Choose a reason for hiding this comment

M0rgan01 commented Jan 9, 2025 •

edited

Loading