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: Always setting public files to true #170

Merged

Conversation

Blagoj5
Copy link

@Blagoj5 Blagoj5 commented May 14, 2023

Problem

There is currently a bug in the code especially on line 46 where publicFIles is always true, e.g

config.publicFiles = false || true; // ALWAYS: true
config.publicFiles = true || true; // ALWAYS: true
config.publicFiles = eval('something else') || true; // throws an error and it was not handled

Solution

Make sure we do proper checking with the new util function setConfigField in case if the config value is string, undefined or an actual boolean. I didn't see a reason to really add tests for this, but if you guys want to do this, I can take care of it.

@Blagoj5
Copy link
Author

Blagoj5 commented May 14, 2023

I can't really tag reviewers, so I'll ping with this comment @Lith @dgmike 😅

@Lith Lith self-requested a review May 15, 2023 06:07
@codecov-commenter
Copy link

codecov-commenter commented May 15, 2023

Codecov Report

Patch coverage: 54.54% and project coverage change: -3.04 ⚠️

Comparison is base (b5fa17e) 81.48% compared to head (067688b) 78.44%.

❗ Current head 067688b differs from pull request most recent head 6f47e5e. Consider uploading reports for the commit 6f47e5e to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #170      +/-   ##
==========================================
- Coverage   81.48%   78.44%   -3.04%     
==========================================
  Files           2        2              
  Lines         108      116       +8     
==========================================
+ Hits           88       91       +3     
- Misses         20       25       +5     
Impacted Files Coverage Δ
lib/provider.js 78.07% <54.54%> (-3.07%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@Lith
Copy link
Collaborator

Lith commented May 15, 2023

Thanks @Blagoj5 ! Could you please signing your commits ? I'm unable to merge without it :/

Signed-off-by: Blagoj Petrov <blagoj@kdsol.net>
@Blagoj5 Blagoj5 force-pushed the fix/always-setting-public-files-to-true branch from 57f296a to 6f47e5e Compare May 15, 2023 08:18
@Blagoj5
Copy link
Author

Blagoj5 commented May 15, 2023

It should be signed now, tell me if you have any problems. I wasn't aware this was an requirement sorry. @Lith

@Lith Lith merged commit 07eb841 into strapi-community:master May 15, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants