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

More helpful error message in checkEnvironment.php #958

Merged
merged 4 commits into from Apr 18, 2019

Conversation

Projects
None yet
2 participants
@amosfolz
Copy link
Contributor

commented Apr 6, 2019

Provide helpful error message in situation where cache session or logs is accidentally deleted or moved.
The existing error was:
image

This adds checkDirectories which provides a more helpful error message:
image

Because I was unable to come up with a more creative way to prevent checkPermissions from returning the generic error message, that check is skipped until the directories actually exist.

amosfolz added some commits Mar 28, 2019

Update CheckEnvironment.php
Fix directory names
@codecov

This comment has been minimized.

Copy link

commented Apr 6, 2019

Codecov Report

Merging #958 into hotfix will increase coverage by 0.22%.
The diff coverage is 0%.

Impacted file tree graph

@@             Coverage Diff              @@
##             hotfix     #958      +/-   ##
============================================
+ Coverage     36.82%   37.05%   +0.22%     
- Complexity     2121     2127       +6     
============================================
  Files           158      159       +1     
  Lines          7212     7233      +21     
============================================
+ Hits           2656     2680      +24     
+ Misses         4556     4553       -3
Impacted Files Coverage Δ Complexity Δ
app/sprinkles/core/src/Util/CheckEnvironment.php 2.27% <0%> (-0.2%) 41 <3> (+3)
...account/src/Repository/PasswordResetRepository.php 0% <0%> (ø) 1% <0%> (ø) ⬇️
...inkles/core/src/Session/DatabaseSessionHandler.php 77.77% <0%> (ø) 3% <0%> (?)
app/sprinkles/core/src/Sprunje/Sprunje.php 38.96% <0%> (+0.49%) 56% <0%> (ø) ⬇️
...les/core/src/ServicesProvider/ServicesProvider.php 58.36% <0%> (+1.63%) 99% <0%> (ø) ⬇️
app/sprinkles/account/src/Database/Models/User.php 60.5% <0%> (+9.24%) 35% <0%> (ø) ⬇️
.../sprinkles/core/src/Session/NullSessionHandler.php 83.33% <0%> (+16.66%) 6% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e82129f...f9a3398. Read the comment docs.

@lcharette
Copy link
Member

left a comment

Logic looks good. Careful with the indentation and overall style of your code. I added a couple of hint in the comments. StyleCi (style checker bot) will be activated on the main repo probably when I'm done with Support package, so better work on this now otherwise the bot will yell at you in your future PR 😛 .

I'll tag this for 4.2.1 release, give it a try and merge when I'm done with the other issue (unless someone else can review it before me 😬 )

if ($this->checkDirectories()) {
$problemsFound = true;
// Skip checkPermissions() if the required directories do not exist.
return $problemsFound;

This comment has been minimized.

Copy link
@lcharette

lcharette Apr 6, 2019

Member

Careful with the indentation here

This comment has been minimized.

Copy link
@amosfolz

amosfolz Apr 6, 2019

Author Contributor

Thanks for the information! I will look over the PSR guides and get more familiar with the UF style guide as well.

];
foreach ($directoryPaths as $directory => $path) {
if ($path == null) {

This comment has been minimized.

Copy link
@lcharette

lcharette Apr 6, 2019

Member

Indentation should be 4 spaces

'success' => false
];
}
else {

This comment has been minimized.

Copy link
@lcharette

lcharette Apr 6, 2019

Member

Brace should be on the same line per PSR-2
https://www.php-fig.org/psr/psr-2/#51-if-elseif-else

@lcharette lcharette added this to the 4.2.x milestone Apr 6, 2019

@lcharette

This comment has been minimized.

Copy link
Member

commented Apr 6, 2019

Related #934

@lcharette lcharette changed the title More helpful error message in checkEnviornment.php More helpful error message in checkEnvironment.php Apr 6, 2019

@lcharette
Copy link
Member

left a comment

Styling should be fixed before this can be merged.

@lcharette lcharette closed this Apr 15, 2019

@lcharette lcharette reopened this Apr 15, 2019

@lcharette lcharette modified the milestones: 4.2.x, 4.2.1 Apr 15, 2019

@lcharette lcharette merged commit 68d030d into userfrosting:hotfix Apr 18, 2019

2 of 3 checks passed

codecov/patch 0% of diff hit (target 36.82%)
Details
codecov/project 37.05% (+0.22%) compared to e82129f
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lcharette

This comment has been minimized.

Copy link
Member

commented Apr 19, 2019

Merged into hotfix for 4.2.1 release. Thanks !

@amosfolz amosfolz deleted the amosfolz:checkenviornment branch Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.