Backend interfaces #8935

rullzer · 2018-03-22T08:20:23Z

This PR introduces interfaces to get rid of implementsActions of the user backends. Using interfaces here is much cleaner and actually what they were designed for.

I also created an abstractbackend that makes sure that backends using this new interface will continue to work as we slowly move over all the backends.

@klada @MorrisJobke how does this look?

klada · 2018-03-22T08:34:27Z

I like this approach. It's much cleaner than my suggestion from PR #8896 and user backend developers now have a clear starting point (which is AbstractBackend). The public abstract backend class still uses the OC\User\Backend namespace, but I think this is okay in this case, as this class is completely under your control.

Of course this also closes #8895.

codecov · 2018-03-22T09:54:15Z

Codecov Report

❗ No coverage uploaded for pull request base (master@874f134). Click here to learn what that means.
The diff coverage is 96.77%.

@@            Coverage Diff            @@
##             master    #8935   +/-   ##
=========================================
  Coverage          ?   51.97%           
  Complexity        ?    25278           
=========================================
  Files             ?     1604           
  Lines             ?    94896           
  Branches          ?     1388           
=========================================
  Hits              ?    49321           
  Misses            ?    45575           
  Partials          ?        0

Impacted Files	Coverage Δ	Complexity Δ
lib/private/User/Database.php	`73.22% <100%> (ø)`	`42 <17> (?)`
lib/public/User/Backend/AbstractBackend.php	`94.44% <94.44%> (ø)`	`9 <9> (?)`

rullzer · 2018-03-22T10:53:06Z

The public abstract backend class still uses the OC\User\Backend namespace, but I think this is okay in this case, as this class is completely under your control.

Yes that is fine. As long as you only use the OCP namespace it is all good.
Note that I also deprecated that method. But we need some time to move all the parts over. And this seems to be the best way I think.

MorrisJobke · 2018-03-22T11:05:21Z

This is so much cleaner 👍 Nice 😄

rullzer · 2018-03-22T11:07:49Z

@MorrisJobke I'll see if I can convert our db backend into this. And if all test still pass then I'd vote to get it in and move all other backens over slowly

ChristophWurst

YESS! Way to go! 🙌

Just a tiny note: since we're using the I prefix for interfaces (I assume someone with a C# or similar background started this), I wonder if we should use A for abstract classes, like ABackend. I'm fine with either.

MorrisJobke · 2018-03-22T13:22:18Z

Just a tiny note: since we're using the I prefix for interfaces (I assume someone with a C# or similar background started this), I wonder if we should use A for abstract classes, like ABackend. I'm fine with either.

Fine with me. @nickvergessen @blizzz @schiessle Opinions?

MorrisJobke · 2018-03-22T14:29:19Z

lib/public/User/Backend/ICheckPasswordBackend.php

+ */
+interface ICheckPasswordBackend {
+	/**
+	 * @since 14.0.0


Could you document the return values and usage for all those methods as well? (just take the ones from the DatabaseBackend

ChristophWurst · 2018-03-22T15:24:05Z

lib/composer/composer/autoload_classmap.php

@@ -302,6 +302,15 @@
    'OCP\\Template' => $baseDir . '/lib/public/Template.php',
    'OCP\\User' => $baseDir . '/lib/public/User.php',
    'OCP\\UserInterface' => $baseDir . '/lib/public/UserInterface.php',
+    'OCP\\User\\Backend\\AbstractBackend' => $baseDir . '/lib/public/User/Backend/AbstractBackend.php',


no up to date with latest changes

ChristophWurst · 2018-03-22T15:24:11Z

lib/composer/composer/autoload_static.php

@@ -332,6 +332,15 @@ class ComposerStaticInit53792487c5a8370acc0b06b1a864ff4c
        'OCP\\Template' => __DIR__ . '/../../..' . '/lib/public/Template.php',
        'OCP\\User' => __DIR__ . '/../../..' . '/lib/public/User.php',
        'OCP\\UserInterface' => __DIR__ . '/../../..' . '/lib/public/UserInterface.php',
+        'OCP\\User\\Backend\\AbstractBackend' => __DIR__ . '/../../..' . '/lib/public/User/Backend/AbstractBackend.php',


no up to date with latest changes

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Because we don't return consistent things yet :( Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

fixed

MorrisJobke · 2018-03-22T15:43:26Z

lib/public/User/Backend/ABackend.php

+	 * @param int $actions The action to check for
+	 * @return bool
+	 */
+	public function implementsActions($actions): bool {


Any reason there is no int type hinted?

Yes because this inherits from the old interfaces. And there is no typehint there.
Adding the typehint will make any userbackend do 💥 (including LDAP).

MorrisJobke · 2018-03-22T15:44:13Z

lib/public/User/Backend/ICheckPasswordBackend.php

+	 * @param string $password The password
+	 * @return string|bool The uid on success false on failure
+	 */
+	public function checkPassword(string $loginName, string $password);


Would it make sense to type hint the success case and expect an exception on failure?

Yes it would. But lets do baby steps. First get this in and then have an issue/PR to discuss further steps and improvements. Because if we change all that we also have to change all the logic that is calling this etc.

MorrisJobke · 2018-03-22T15:44:23Z

lib/public/User/Backend/ICountUsersBackend.php

+	 *
+	 * @return int|bool The number of users on success false on failure
+	 */
+	public function countUsers();


Same exception handling here.

MorrisJobke · 2018-03-22T15:44:46Z

lib/public/User/Backend/IGetHomeBackend.php

+	 * @param string $uid the username
+	 * @return string|bool Datadir on success false on failure
+	 */
+	public function getHome(string $uid);


Same exception handling here.

MorrisJobke · 2018-03-22T15:45:36Z

lib/public/User/Backend/ICreateUserBackend.php

+	 * @param string $password The password of the new user
+	 * @return bool
+	 */
+	public function createUser(string $uid, string $password): bool;


I expect it to succeed or to throw an exception. So there should not be any return value.

I can't change that without chaning the whole backend logic as well. It will happen but baby steps.

MorrisJobke · 2018-03-22T15:45:48Z

lib/public/User/Backend/ISetDisplayNameBackend.php

+	 * @param string $displayName The new display name
+	 * @return bool
+	 */
+	public function setDisplayName(string $uid, string $displayName): bool;


I expect it to succeed or to throw an exception. So there should not be any return value.

MorrisJobke · 2018-03-22T15:45:54Z

lib/public/User/Backend/ISetPasswordBackend.php

+	 * @param string $password The new password
+	 * @return bool
+	 */
+	public function setPassword(string $uid, string $password): bool;


I expect it to succeed or to throw an exception. So there should not be any return value.

nickvergessen · 2018-03-26T11:40:24Z

Could have also started to use full words (Interface and Abstract) instead of introducing A now ;)

rullzer added enhancement 2. developing Work in progress technical debt labels Mar 22, 2018

rullzer added this to the Nextcloud 14 milestone Mar 22, 2018

rullzer mentioned this pull request Mar 22, 2018

Move constants required by UserInterface into interface definition #8896

Closed

rullzer force-pushed the backend_interfaces branch from aec2e31 to 37ac8f5 Compare March 22, 2018 09:54

rullzer requested review from nickvergessen, MorrisJobke, icewind1991, ChristophWurst and juliushaertl March 22, 2018 12:31

rullzer added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Mar 22, 2018

ChristophWurst approved these changes Mar 22, 2018

View reviewed changes

icewind1991 approved these changes Mar 22, 2018

View reviewed changes

MorrisJobke previously requested changes Mar 22, 2018

View reviewed changes

ChristophWurst previously requested changes Mar 22, 2018

View reviewed changes

rullzer added 7 commits March 22, 2018 16:32

Add interfaces for user backend functionality

226439a

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Add abstract bakend to still be backwards compatible with the old logic

355abbc

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Fix phpdoc

140e7b6

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Deprecate implementsActions

c30b9da

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Remove type declarations where not yet possible

5aa0d83

Because we don't return consistent things yet :( Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Move Database backend over to new User/Backend interfaces

cbd2be5

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Move to ABackend

471272d

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer added 2 commits March 22, 2018 16:32

Document parameters

b723a2b

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

Update autoloader

f5ce253

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>

rullzer force-pushed the backend_interfaces branch from 385374d to f5ce253 Compare March 22, 2018 15:32

MorrisJobke reviewed Mar 22, 2018

View reviewed changes

MorrisJobke approved these changes Mar 22, 2018

View reviewed changes

rullzer merged commit 34cb8ea into master Mar 23, 2018

ChristophWurst deleted the backend_interfaces branch March 23, 2018 07:48

rullzer mentioned this pull request Mar 23, 2018

User backends (apps) using private constants from OC\User\Backend #8895

Closed

MorrisJobke mentioned this pull request Apr 23, 2018

Fixed disabled users query #9261

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Backend interfaces #8935

Backend interfaces #8935

rullzer commented Mar 22, 2018

klada commented Mar 22, 2018

codecov bot commented Mar 22, 2018 •

edited

Loading

rullzer commented Mar 22, 2018

MorrisJobke commented Mar 22, 2018

rullzer commented Mar 22, 2018

ChristophWurst left a comment

MorrisJobke commented Mar 22, 2018

MorrisJobke Mar 22, 2018

rullzer Mar 22, 2018

ChristophWurst Mar 22, 2018

ChristophWurst Mar 22, 2018

MorrisJobke Mar 22, 2018

rullzer Mar 22, 2018

MorrisJobke Mar 22, 2018

MorrisJobke Mar 22, 2018

rullzer Mar 22, 2018

MorrisJobke Mar 22, 2018

MorrisJobke Mar 22, 2018

MorrisJobke Mar 22, 2018

MorrisJobke Mar 22, 2018

rullzer Mar 22, 2018

MorrisJobke Mar 22, 2018

MorrisJobke Mar 22, 2018

MorrisJobke Mar 22, 2018

nickvergessen commented Mar 26, 2018

Backend interfaces #8935

Backend interfaces #8935

Conversation

rullzer commented Mar 22, 2018

klada commented Mar 22, 2018

codecov bot commented Mar 22, 2018 • edited Loading

Codecov Report

rullzer commented Mar 22, 2018

MorrisJobke commented Mar 22, 2018

rullzer commented Mar 22, 2018

ChristophWurst left a comment

Choose a reason for hiding this comment

MorrisJobke commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Mar 26, 2018

codecov bot commented Mar 22, 2018 •

edited

Loading