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

&run does not work with all hooks anymore since refactoring #16908

Closed
LukasReschke opened this issue Jun 12, 2015 · 8 comments · Fixed by #19545
Closed

&run does not work with all hooks anymore since refactoring #16908

LukasReschke opened this issue Jun 12, 2015 · 8 comments · Fixed by #19545

Comments

@LukasReschke
Copy link
Member

Regression since a longer time, as described at #16750 (comment):

\OCP\Util::connectHook('OC_User', 'pre_createUser', '\OCA\MyApp\MyClass', 'createUser');

And in MyClass do something like:

<?php

namespace \OCA\MyApp;

class MyClass {
    public function createUser(&$params) {
        // Stop running – that should suffice…
        $params['run'] = false;
        // FIXME: Huge hack around a bug…
        exit();
    }
}

Option 1: Fix it.
Option 2: Fix the documentation in the user class.

Thoughts?

@PVince81
Copy link
Contributor

The problem might be in the code that invokes the hook.
If that code ignores the value of "run", then the code flow will continue.

This is not an issue specific to the hook system, but rather to the hook caller.

Need to dig into the users code.

@LukasReschke LukasReschke added this to the 8.2-next milestone Jun 12, 2015
@PVince81
Copy link
Contributor

Ok problem is here:

\OC_Hook::emit('OC_User', 'pre_createUser', array('run' => true, 'uid' => $uid, 'password' => $password));

"run" is always true.

But even if we passed by reference, the other "new" hook system doesn't have a way to cancel the hook AFAIK. @icewind1991 what do you think ?

@LukasReschke
Copy link
Member Author

But even if we passed by reference, the other "new" hook system doesn't have a way to cancel the hook AFAIK. @icewind1991 what do you think ?

So basically this also affects all the other hooks below as well and we're doomed anyways? 🙈

@icewind1991
Copy link
Contributor

Option 2: Fix the documentation in the user class.

imo having hooks cancel the operation was a terrible design decision

@PVince81
Copy link
Contributor

Any update ? Fixing the documentation might be safer...

@ghost
Copy link

ghost commented Sep 22, 2015

#16908 (comment) maybe, but we are where we are for now. @carlaschroder document?

@carlaschroder
Copy link

I'm happy to proofread, but this looks like something a real developer needs to write.

@ghost
Copy link

ghost commented Oct 1, 2015

@PVince81 @LukasReschke suggestions?

@ghost ghost added the sev2-high label Oct 1, 2015
@lock lock bot locked as resolved and limited conversation to collaborators Aug 8, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants