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 recursive check of updated files #16765

Merged
merged 1 commit into from Jan 8, 2020

Conversation

@Darhazer
Copy link
Contributor

Darhazer commented Dec 11, 2019

The return value of the recursive call is not used, so even if it
reports changed files in subfolders, they won't be returned at the end.

The URL to the xml file was incorrect, so it never got to the recursive
call in the first place.

Questions Answers
Branch? develop
Description? In version 1.7 the RequiredFilesChecker is broken. See the PR description for details.
Type? bug fix
Category? BO
BC breaks? No
Deprecations? No
Fixed ticket? N/A
How to test? Modify a file in the core and go to Back Office -> Advanced-> System information. Your modified file should be on the list. Note: there is no XML file for the develop version, so you should cherry-pick this in an already released version to test (or just modify the AppKernel::VERSION constant to something released, e.g. 1.7.6.2)

This change is Reviewable

@Darhazer Darhazer requested a review from PrestaShop/prestashop-core-developers as a code owner Dec 11, 2019
@prestonBot

This comment has been minimized.

Copy link
Collaborator

prestonBot commented Dec 11, 2019

Hello @Darhazer!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

@Darhazer Darhazer force-pushed the Darhazer:fix-recursive-file-checker branch 2 times, most recently from fb40c88 to 1fa4083 Dec 11, 2019
The return value of the recursive call is not used, so even if it
reports changed files in subfolders, they won't be returned at the end.

The URL to the xml file was incorrect, so it never got to the recursive
call in the first place.
@Darhazer Darhazer force-pushed the Darhazer:fix-recursive-file-checker branch from 1fa4083 to f42b1be Dec 13, 2019
@PierreRambaud PierreRambaud added this to the 1.7.7.0 milestone Dec 16, 2019
@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Dec 31, 2019

hello @Darhazer

Thank you for your contribution.

I just test the PR and the return value indicated that there is no file change even if I change a file.

@sarahdib sarahdib self-assigned this Dec 31, 2019
@Darhazer

This comment has been minimized.

Copy link
Contributor Author

Darhazer commented Dec 31, 2019

Hello @sarahdib

Did you make sure that AppKernel::VERSION points to a version that has an XML file with the checksums? As in dev mode / 1.7.7 you may not have a valid URL.

Unfortunately, there are a few things that might fail with loading the XML and there is no error message shown to the user. This is a separate improvement that could be made, but a decision regarding the UI and the wording should be made.

So to test this PR, please make sure it loads the xml file successfully and that the modified file is present in the list of files in the XML.

@sarahdib

This comment has been minimized.

Copy link

sarahdib commented Jan 8, 2020

@Darhazer thank you for your PR it's ok and thanks to @Progi1984 who help me with the test

@Progi1984 Progi1984 merged commit 60a1782 into PrestaShop:develop Jan 8, 2020
2 checks passed
2 checks passed
PrettyCI Code formatting
Details
Travis CI - Pull Request Build Passed
Details
@Progi1984

This comment has been minimized.

Copy link
Contributor

Progi1984 commented Jan 8, 2020

Thanks @Darhazer for your first contribution 🎉

@Darhazer Darhazer deleted the Darhazer:fix-recursive-file-checker branch Jan 8, 2020
Progi1984 added a commit that referenced this pull request Jan 10, 2020
Fix recursive check of updated files - backport of #16765
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.