terminal42 / contao-mailusername

MIT License
7 stars 5 forks source link

Fix registration #26

Closed fritzmg closed 2 years ago

fritzmg commented 2 years ago

The fix in 426aa78d23f36cf1d57c49a4f1c5cabb18171669 was wrong as this causes the callback to not be executed at all anymore in the front end.

Opening this as a draft as @inndividuell will still test the changes.

aschempp commented 2 years ago

shouldn't we change line 60 then as well, since we should never modify $varValue?

fritzmg commented 2 years ago

Line 60 is there due to https://github.com/terminal42/contao-mailusername/issues/15 as far as I can see. But I think we need to test all cases again.

ameotoko commented 2 years ago

This still does not work in front end because $dc is null:

https://github.com/terminal42/contao-mailusername/blob/426aa78d23f36cf1d57c49a4f1c5cabb18171669/src/EventListener/MailUsernameListener.php#L74-L77

ErrorException: Warning: Attempt to read property "id" on null
    in /vendor/terminal42/contao-mailusername/src/EventListener/MailUsernameListener.php:77

The record is never even created, because the callback does not return anything in this case.

ameotoko commented 2 years ago

https://github.com/terminal42/contao-mailusername/blob/426aa78d23f36cf1d57c49a4f1c5cabb18171669/src/EventListener/MailUsernameListener.php#L74-L77

This additional change fixes it for me:

+++ if (null !== $dc) {
        $this->connection->update(
            'tl_member',
            ['username' => $strValue],
            ['id' => $dc->id]
    );
+++ }
fritzmg commented 2 years ago

@ameotoko tested and verified the changes in pretty much every case, so this should be RTM.

aschempp commented 2 years ago

I've fixed this differently in 57fd76ca27d80f41651bdca3edd65df032ab9095, I hope that works as well 🙈

fritzmg commented 2 years ago

I've fixed this differently in 57fd76c, I hope that works as well 🙈

This will now create tl_member entries with an empty username in cases like https://github.com/terminal42/contao-mailusername/pull/21

You need to make sure the registration form does not validate in case the e-mail address already exists (as a username) in the database. This has now been disabled by your commit.

aschempp commented 2 years ago

There is no way to bypass the registration in the createNewUser hook, except for throwing an exception?

fritzmg commented 2 years ago

createNewUser is too late, ist must be done in the save_callback of the email field.

aschempp commented 2 years ago

Fixed in 7e1ad89b63e0ed15372acea5547b28fc65b6a484