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

Capabilities manager #15093

Merged
merged 4 commits into from
Aug 10, 2015
Merged

Capabilities manager #15093

merged 4 commits into from
Aug 10, 2015

Conversation

rullzer
Copy link
Contributor

@rullzer rullzer commented Mar 21, 2015

Implementation of proposal in #14805

The first commit adds the capabilities manager. There now is a capabilities manager where apps can register them self. The capabilities are then all collected in 1 place by iterating over all the apps that have registered a capabilities manager.

The second commit moves the files app to the shiny new capabilities manager.

Comments and suggestions are very welcome. As this would help to get #13964 in. Which would greatly improve the client UX.

Unit tests will of course follow once I have addressed the comments.

CC: @DeepDiver1975 @PVince81 @Raydiation @LukasReschke

@rullzer rullzer added this to the 8.1-current milestone Mar 21, 2015
@RobinMcCorkell
Copy link
Member

Capabilitie -> Capability (found in multiple places)

@rullzer
Copy link
Contributor Author

rullzer commented Mar 21, 2015

Capabilitie -> Capability (found in multiple places)

Thanks, fixed.

@rullzer rullzer changed the title Capabilities manager [WIP] Capabilities manager Mar 21, 2015
/**
* Register capabilities
*/
$server->getCapabilitiesManager()->registerCapability(function() use ($container) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this block for ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok, got it... I mixed up CapabilitiesController and CapabilitiesManager.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To register the capabilities with the manager. Else how would it know where to look?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove the use, you dont use it anymore

@PVince81
Copy link
Contributor

Great stuff!

You might want to adapt the other apps next (files_sharing, files_versions, files_trashbin) to also use this.

@rullzer
Copy link
Contributor Author

rullzer commented Mar 23, 2015

@PVince81 thanks.

Yeah those are on my list to adapt. But wanted feedback first.

I also plan to print a warning to the log if somebody tries to register a capabilities route instead of the capabilities manager.

@PVince81
Copy link
Contributor

Please rebase.

Is this ready for test/review ? If yes, please remove the WIP 😄

@rullzer rullzer changed the title [WIP] Capabilities manager Capabilities manager Mar 27, 2015
@PVince81
Copy link
Contributor

I guess this is not yet compatible with the old OCS URLs, isn't it ?
Would be good to make it so that the old OCS capabilities URL uses the capabilities manager.

@PVince81
Copy link
Contributor

(I haven't tested yet)

);


$result['capabilities'] = array_merge_recursive($result['capabilities'], \OC::$server->getCapabilitiesManager()->getCapabilities());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@PVince81 that is actually what happens here. since we do yet have OCS routes in the appframework.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the existing capa... also in a Class:
'pollinterval' => OC_Config::getValue('pollinterval', 60)
so people will stop hard coding it

@MorrisJobke
Copy link
Contributor

12:12:45 There was 1 error:
12:12:45 
12:12:45 1) Test_Files_Sharing_Api::testUpdateShareExpireDate
12:12:45 Doctrine\DBAL\Exception\UniqueConstraintViolationException: An exception occurred while executing 'INSERT INTO oc_appconfig (appid, configkey, configvalue) VALUES (?, ?, ?)' with params ["core", "shareapi_default_expire_date", "yes"]:
12:12:45 
12:12:45 SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: oc_appconfig.appid, oc_appconfig.configkey
12:12:45 
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractSQLiteDriver.php:48
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:116
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:996
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/db/connection.php:149
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:696
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/appconfig.php:191
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/allconfig.php:165
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/apps/files_sharing/tests/api.php:1036
12:12:45 
12:12:45 Caused by
12:12:45 Doctrine\DBAL\Driver\PDOException: SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: oc_appconfig.appid, oc_appconfig.configkey
12:12:45 
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:93
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:989
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/db/connection.php:149
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:696
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/appconfig.php:191
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/allconfig.php:165
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/apps/files_sharing/tests/api.php:1036
12:12:45 
12:12:45 Caused by
12:12:45 PDOException: SQLSTATE[23000]: Integrity constraint violation: 19 UNIQUE constraint failed: oc_appconfig.appid, oc_appconfig.configkey
12:12:45 
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOStatement.php:91
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:989
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/db/connection.php:149
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:696
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/appconfig.php:191
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/lib/private/allconfig.php:165
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/apps/files_sharing/tests/api.php:1036
12:12:45 
12:12:45 --
12:12:45 
12:12:45 There was 1 failure:
12:12:45 
12:12:45 1) Test_Share::testClearExpireDateWhileEnforced
12:12:45 Failed asserting that false is true.
12:12:45 
12:12:45 /var/jenkins/workspace/pull-request-analyser-ng-simple/label/SLAVE/tests/lib/share/share.php:1085

@rullzer
Copy link
Contributor Author

rullzer commented Mar 28, 2015

Jenkins is happy #15278
Commits squashed. And rebased.
Please review!

@MorrisJobke
Copy link
Contributor

cc @LukasReschke @schiesbn

@rullzer
Copy link
Contributor Author

rullzer commented Jul 23, 2015

@nickvergessen thanks! Fixed your comments.
Throwing the exception makes sense.

if ($c instanceof ICapability) {
$capabilities = array_replace_recursive($capabilities, $c->getCapabilities());
} else {
throw new NotACapabilityException();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just throw \InvalidArgumentException
http://php.net/manual/de/class.invalidargumentexception.php

throw new \InvalidArgumentException('The given Capability does not implement the ICapability interface');

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

makes more sense. thanks.
Fixed

@rullzer
Copy link
Contributor Author

rullzer commented Jul 23, 2015

Hopefully the final jenkins PR at #15093

@jospoortvliet
Copy link

That bot isn't easy to satisfy, is it ;-)

@rullzer
Copy link
Contributor Author

rullzer commented Jul 23, 2015

Jenkins is happy #15093
Review and lets get this in!

@rullzer
Copy link
Contributor Author

rullzer commented Jul 27, 2015

@ghost
Copy link

ghost commented Aug 10, 2015

💣 Test FAILed. 💣
nooo432

* This should allow the capabilities to be intergrated into the
appframework
* Unit tests
* Throw exception if closure does not return ICapability instance
* Files
* Files_Sharing
* Files_Trashbin
* Files_Versions
* Register OCP\Capability\IManager at DIContainer
* Add register capabilities to appframework
* Register capabilities in DI way
* Make unit test pass again
* Remove CapabiltiesManager from OCP
@ghost
Copy link

ghost commented Aug 10, 2015

💣 Test FAILed. 💣
nooo432

@scrutinizer-notifier
Copy link

A new inspection was created.

@rullzer
Copy link
Contributor Author

rullzer commented Aug 10, 2015

@owncloud-bot retest this please

@ghost
Copy link

ghost commented Aug 10, 2015

🚀 Test PASSed.🚀
chuck

@PVince81
Copy link
Contributor

  % curl http://root:admin@localhost/owncloud/ocs/v1.php/cloud/capabilities\?format\=json | jsonlint -p -
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   455  100   455    0     0   1813      0 --:--:-- --:--:-- --:--:--  1820
{
  "ocs": {
    "meta": {
      "status": "ok",
      "statuscode": 100,
      "message": null
    },
    "data": {
      "version": {
        "major": 8,
        "minor": 2,
        "micro": 0,
        "string": "8.2 pre alpha",
        "edition": ""
      },
      "capabilities": {
        "core": {
          "pollinterval": 60
        },
        "files": {
          "bigfilechunking": true,
          "undelete": true,
          "versioning": true
        },
        "files_sharing": {
          "public": {
            "enabled": true,
            "password": {
              "enforced": true
            },
            "expire_date": {
              "enabled": true,
              "days": "7",
              "enforced": true
            },
            "send_mail": true
          },
          "user": {
            "send_mail": true
          },
          "resharing": true
        }
      }
    }
  }
}

Looks good to me 👍

@ghost
Copy link

ghost commented Aug 10, 2015

🚀 Test PASSed.🚀
chuck

@rullzer
Copy link
Contributor Author

rullzer commented Aug 10, 2015

@Raydiation can I assume your 👍 is still valid?

@BernhardPosselt
Copy link
Contributor

yes

rullzer added a commit that referenced this pull request Aug 10, 2015
@rullzer rullzer merged commit c2856c0 into owncloud:master Aug 10, 2015
@rullzer rullzer deleted the capabilities_manager branch August 10, 2015 18:33
@lock lock bot locked as resolved and limited conversation to collaborators Aug 11, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants