getgrav / grav-plugin-login

Grav Login Plugin
http://getgrav.org
MIT License
44 stars 54 forks source link

onUserLoginRegisterData event is not being triggered. #155

Closed finanalyst closed 6 years ago

finanalyst commented 6 years ago

I am trying to get the onUserLoginRegisterData event, which is listed in the Login README.md file. It does not trigger. (Rather than continuing this question where I first mentioned it in the Debug issue, I am opening a new more specific issue).

Steps to replicate issue: I created a clean Grav site with Admin and only added devtools. I am using a server running Ubuntu 17.10.

I created a new plugin 'login-test' with devtools CLI.

This is the contents of pages/03.test/default.md:

---
access:
  site.login: true
---

This is the partial content of plugins/login-test/login-test.php:

    public function onPluginsInitialized()
    {
        if ($this->isAdmin()) {
            return;
        }
        $this->enable([
            'onUserLogin' => ['onUserLoginRegisterData', 0]
        ]);
    }
    public function onUserLoginRegisterData(Event $e)
    {
        dump($e);
        exit;
    }

When accessing the test site, then clicking on the Test page, I get the default login screen. I put a valid user in, then I get a dump of all the event data - as expected.

Next I change the enable parameter to:

        $this->enable([
            'onUserLoginRegisterData' => ['onUserLoginRegisterData', 0]
        ]);

After logging out and logging in, I get the Login screen, and after putting in user/password, I only get the contents of default.md. This is not expected. Even if $e is null, the dump routine would have shown 'null'.

I inspected getgrav/grav-plugin-login/blob/develop/login.php, and found that unlike other Login Events listed in the README, onUserLoginRegisterData is not enabled (around line 70). I do not know whether this is the issue. I added onUserRegisterData to my local copy of login.php, and pointed it to a stub public function stub() {}. However, these changes still did not trigger onUserLoginRegisterData.

The onUserLoginRegisterData Event is fired on line 730.

I can work around this limitation using onUserLogin, but I can't see why this Event callback is not working.

rhukster commented 6 years ago

Will investigate!

mahagr commented 6 years ago

I quickly checked this out and looks like that onUserLoginRegisterData only gets fired when a new user gets created by login controller. So it doesn't get fired on login, in that case, you should be using onUserLogin event instead.

finanalyst commented 6 years ago

Firing onUserRegisterData only for new users would explain the observed behaviour.

I have been using onUserLogin.

Unless there is another use case for onUserRegisterData for all users, which cannot otherwise be accommodated by onUserLogin, may I suggest that the routine is renamed to onNewUserRegisterData, which would be self-documenting? (onUserRegisterData might be still be fired in an undocumented manner, so as not to break anything that relies on it)

mahagr commented 6 years ago

Unfortunately, this onUserRegisterData event predates the new events and has been there a long time, so it cannot be changed without potentially breaking existing sites. @rhukster ?

finanalyst commented 6 years ago

Actually, I thought it would be extremely easy, since the $grav->fireEvent('onUserRegisterData',...) is only called once. So just add the line $grav->fireEvent('onNewUserRegisterData',...) on the next line. Then in the documentation refer only to onNewUserRegisterData. The old call will still be fired, thus making sure legacy software will not break.

However, I don't know what effect this will have on run time.

rhukster commented 6 years ago

This event is meant only for registering new users. I don't see any need to change it. "Registering" users is the process of creating new users, and this is the same semantics we use throughout the plugin.