Better error message when Composer files are missing. #12974

Merged
merged 1 commit into from Feb 9, 2017

Projects

None yet

6 participants

@ibennetch
Contributor

Fixes #12972

Signed-off-by: Isaac Bennetch bennetch@gmail.com

Before submitting pull request, please check that every commit:

  • Has proper Signed-Off-By
  • Has commit message which describes it
  • Is needed on it's own, if you have just minor fixes to previous commits, you can squash them
  • Any new functionality is covered by tests
@ibennetch ibennetch requested a review from nijel Feb 9, 2017
@ibennetch
Contributor

I believe we can't translate these strings because motranslator hasn't been loaded yet.

libraries/common.inc.php
+ . 'Most likely you did not run Composer to '
+ . '<a href="https://docs.phpmyadmin.net/en/latest/setup.html#installing-from-git">install library files</a>.'
+ );
+}
@nijel
nijel Feb 9, 2017 Member

I'd rather do it if (! file_exist || ! is _readable) { die() }; so that require_once is not behind condition (or it should be turned into include_once, what I think does not make sense here).

@OlafvdSpek
OlafvdSpek Feb 9, 2017

file_exists seems redundant if you call is_readable..

@nijel
nijel Feb 9, 2017 Member

True, also it probably should have @ in front to avoid showing PHP errors.

@OlafvdSpek
OlafvdSpek Feb 9, 2017

What errors?
PHP doesn't error / warn for is_readable does it?

@poush
poush Feb 9, 2017 Contributor

Yes, from php.net "Upon failure, an E_WARNING is emitted."

@OlafvdSpek
OlafvdSpek Feb 9, 2017

A non-readable file is not a failure for this function.

@nijel
nijel Feb 9, 2017 Member

Non-readable file is not, but file when stat() fails, it emits warning.

@OlafvdSpek
OlafvdSpek Feb 9, 2017

printf("%x\n", error_reporting()); is_readable('a'); outputs 57ff and doesn't emit any warnings.

@OlafvdSpek
OlafvdSpek Feb 9, 2017 edited

When would stat() fail? And why would you want to suppress the warning?

@nijel
nijel Feb 9, 2017 Member

For example in case of open_basedir restrictions (see #11951). We want to suppress it to avoid full path disclosure vulnerability.

@codecov-io
codecov-io commented Feb 9, 2017 edited

Codecov Report

Merging #12974 into QA_4_7 will decrease coverage by -0.01%.

@@            Coverage Diff             @@
##           QA_4_7   #12974      +/-   ##
==========================================
- Coverage   54.24%   54.24%   -0.01%     
==========================================
  Files         466      466              
  Lines       69556    69562       +6     
==========================================
  Hits        37733    37733              
- Misses      31823    31829       +6

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 99e4e0d...8db5de0. Read the comment docs.

@ibennetch ibennetch Better error message when Composer files are missing.
Fixes #12972

Signed-off-by: Isaac Bennetch <bennetch@gmail.com>
8db5de0
@ibennetch
Contributor

I've updated the pull request with all of your suggestions. It now suppresses warnings and only tests with is_readable().

Thanks for the feedback.

@nijel nijel merged commit 049382b into phpmyadmin:QA_4_7 Feb 9, 2017

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@nijel
Member
nijel commented Feb 9, 2017

Thanks, merged.

@nijel nijel self-assigned this Feb 9, 2017
@nijel nijel added this to the 4.7.0 milestone Feb 9, 2017
@jublo
jublo commented Feb 11, 2017

I loved this when I got to set up a Git-powered instance just today. It helped me figure the error in an instant.

@ibennetch
Contributor

Great, I'm glad it helped!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment