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

Validate appinfo against appstore schema #8232

Merged
merged 3 commits into from Feb 16, 2018

Conversation

@nickvergessen
Copy link
Member

nickvergessen commented Feb 7, 2018

Fix #8007

Schema changes for app store in nextcloud/appstore#547

@rullzer

This comment has been minimized.

Copy link
Member

rullzer commented Feb 8, 2018

CI is not happy about this

@nickvergessen

This comment has been minimized.

Copy link
Member

nickvergessen commented Feb 8, 2018

Nextcloud is not installed - only a limited number of commands are available

[Doctrine\DBAL\DBALException]
Failed to connect to the database: An exception occured in driver: SQLSTATE
[HY000] [14] unable to open database file

app:check-code [-c|--checker CHECKER] [--skip-checkers] [--skip-validate-info] [--]

PHP Warning: fileperms(): stat failed for /drone/src/github.com/nextcloud/server/data/nextcloud.log in /drone/src/github.com/nextcloud/server/lib/private/Log/File.php on line 139

sounds like disk is full?

@BernhardPosselt

This comment has been minimized.

Copy link
Member

BernhardPosselt commented Feb 9, 2018

Schema updated fyi

@nickvergessen nickvergessen force-pushed the bugfix/8007/validate-appinfo-against-appstore-schema branch from 25457ad to 173662f Feb 9, 2018

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 9, 2018

Codecov Report

Merging #8232 into master will increase coverage by 0.07%.
The diff coverage is 72.41%.

@@             Coverage Diff              @@
##             master    #8232      +/-   ##
============================================
+ Coverage     51.71%   51.79%   +0.07%     
+ Complexity    25404    25383      -21     
============================================
  Files          1600     1600              
  Lines         95135    95087      -48     
  Branches       1377     1377              
============================================
+ Hits          49198    49249      +51     
+ Misses        45937    45838      -99
Impacted Files Coverage Δ Complexity Δ
lib/private/App/AppManager.php 96.1% <ø> (ø) 58 <0> (ø) ⬇️
core/Command/App/CheckCode.php 0% <0%> (ø) 33 <1> (-16) ⬇️
core/register_command.php 0% <0%> (ø) 0 <0> (ø) ⬇️
lib/private/App/CodeChecker/InfoChecker.php 80.64% <84%> (+1.92%) 10 <10> (-5) ⬇️
lib/private/Security/CertificateManager.php 91% <0%> (-3%) 39% <0%> (ø)
lib/private/Server.php 83.28% <0%> (+0.09%) 282% <0%> (ø) ⬇️
lib/private/Files/ObjectStore/Swift.php 49.61% <0%> (+49.61%) 45% <0%> (ø) ⬇️

@nickvergessen nickvergessen force-pushed the bugfix/8007/validate-appinfo-against-appstore-schema branch from 173662f to 568fe83 Feb 14, 2018

@nickvergessen

This comment has been minimized.

Copy link
Member

nickvergessen commented Feb 14, 2018

@MorrisJobke @rullzer rebased, please review

@nickvergessen nickvergessen force-pushed the bugfix/8007/validate-appinfo-against-appstore-schema branch from c0e27c9 to ec8e002 Feb 15, 2018

$infoChecker->listen('InfoChecker', 'mandatoryFieldMissing', function($key) use ($output) {
$output->writeln("<error>Mandatory field missing: $key</error>");
// Can not inject because of circular dependency
$infoChecker = new InfoChecker(\OC::$server->getAppManager());

This comment has been minimized.

@nickvergessen

nickvergessen Feb 15, 2018

Member

Okay, this breaks the tests, because it depends on AppConfig which depends on IDBConnection, although the instance is not installed.
Easy fix would be to make this command not available anymore on non-installed instances.

Another idea would be to split the isShipped/isAlwaysEnabled methods out into another class which can be used without database.

Preferences? @rullzer @MorrisJobke

This comment has been minimized.

@MorrisJobke

MorrisJobke Feb 15, 2018

Member

I would tend to have it in a separate class/trait and pull it in the app manager then. Something like we did with IConfig and SystemConfig. But this is only my opinion.

This comment has been minimized.

@nickvergessen

nickvergessen Feb 16, 2018

Member

Hmm instead of that I copied the "read shipped.json and check for existance" now to this class directly. Seems easier and good enough for now.

This comment has been minimized.

@juliushaertl

juliushaertl Feb 16, 2018

Member

The InfoChecker doesn't need the AppManager anymore. 😉

This comment has been minimized.

@juliushaertl

juliushaertl Feb 16, 2018

Member

I don't like the duplicate code that much. Can't we just use the InfoChecker in the AppManager now?

nickvergessen added some commits Feb 7, 2018

Validate the info.xml against the appstore schema file
Signed-off-by: Joas Schilling <coding@schilljs.com>
Fix info.xml files of shipped apps
Signed-off-by: Joas Schilling <coding@schilljs.com>

@nickvergessen nickvergessen force-pushed the bugfix/8007/validate-appinfo-against-appstore-schema branch from 73d884b to f095001 Feb 16, 2018

$infoChecker->listen('InfoChecker', 'mandatoryFieldMissing', function($key) use ($output) {
$output->writeln("<error>Mandatory field missing: $key</error>");
// Can not inject because of circular dependency
$infoChecker = new InfoChecker(\OC::$server->getAppManager());

This comment has been minimized.

@juliushaertl

juliushaertl Feb 16, 2018

Member

The InfoChecker doesn't need the AppManager anymore. 😉

$infoChecker->listen('InfoChecker', 'mandatoryFieldMissing', function($key) use ($output) {
$output->writeln("<error>Mandatory field missing: $key</error>");
// Can not inject because of circular dependency
$infoChecker = new InfoChecker(\OC::$server->getAppManager());

This comment has been minimized.

@juliushaertl

juliushaertl Feb 16, 2018

Member

I don't like the duplicate code that much. Can't we just use the InfoChecker in the AppManager now?

App manager is not needed anymore
Signed-off-by: Joas Schilling <coding@schilljs.com>
continue;
$schema = \OC::$SERVERROOT . '/resources/app-info.xsd';
try {
if ($this->isShipped($appId)) {

This comment has been minimized.

@nickvergessen

nickvergessen Feb 16, 2018

Member

I don't like the duplicate code that much. Can't we just use the InfoChecker in the AppManager now?

@juliushaertl sorry, but that makes no sense from API point of view, also we are actually talking about 2 lines of code which read a JSON file. not worth a lot of troubles in my opinion. I can also remove all the function overhead which is just there to keep the code in line with the app manager.

$content = json_decode(file_get_contents(\OC::$SERVERROOT . '/core/shipped.json'), true);
if (\in_array($appId, $content['shippedApps'], true)) {

This comment has been minimized.

@juliushaertl

juliushaertl Feb 16, 2018

Member

Ok, fine by me then. I guess we should have a subset of the AppManager available for using without any DB setup in the future.

This comment has been minimized.

@MorrisJobke

MorrisJobke Feb 16, 2018

Member

We also thought a lot about where to put it but in the end for those few lines of code it was just overkill to create another layer of abstraction. ;)

@MorrisJobke MorrisJobke merged commit 57e0881 into master Feb 16, 2018

1 of 2 checks passed

continuous-integration/drone/pr the build failed
Details
Scrutinizer 3 updated code elements
Details

@MorrisJobke MorrisJobke deleted the bugfix/8007/validate-appinfo-against-appstore-schema branch Feb 16, 2018

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