Improve Event Filters

[michael-e]

Imagine a frontend form with the following fields:

• old password
• new password
• new password (again, please!)

Currently, you can not use such a form with Members. Sure, you may use a native Symphony event to save to the Members section, but the available filters will not provide what you need. Let me elaborate a bit.

• Members is missing a Members: Require Password pre-save filter. Such a filter might allow to check for the (old) password before saving the new password. And it might be useful in other contexts, too. ("Hey, you are doing something severe! Please enter your password, just to be sure.") If the correct password is posted, the filter returns true, otherwise false (which terminates the event).

  As a sidenote: Generally, requiring the password (although someone is already logged in) has become more and more popular. I suspect this is the reaction to the fact that many people never log out from a website or web app and leave "open accounts" unattended. Those "extra password requests" limit the possible damage of this bad user behaviour.

• Members has a Members: Update Password filter, but this one does too much, because it calls filter_UpdatePassword as well as filter_UpdatePasswordLogin. The first function allows to save an empty password, which is useful for different Members forms, but not for the simple "pure password form" described above. We don't want to allow empty new passwords in this case. The second function does what we desire: Log the Member in with the new password.

• There is also the Members: Login filter which only calls filter_UpdatePasswordLogin, so it would be perfect for our secenario. But this filter is undocumented, and it is only available if no "authentication" type field is used. IMHO the latter is based on a misunderstanding in a discussion from 2011 (Login after registration #44) – @brendo talked about requiring activation before logging a Member in. I don't see a reason to not always reveal this filter. @brendo: What do you think? Am I missing something?

So here is my suggestion:

1. Always show the Members: Login filter.
2. Add documentation for this filter to the README.
3. Add a Members: Require Password (pre-save) filter.
4. Add documentation for this filter to the README.

I'd like to make sure that I am on the right track before sending any pull requests. So please, tell me what you think!

Also, regarding the naming of the new filter, would you prefer Members: Require Password or maybe Members: Check Password? Other ideas?

[michael-e]

This is nearly finished, and I personally would like to see it in the next release. Unfortunately there is no feedback still. So should I send a pull request (to discuss)?

[michael-e]

BTW: The name of the filter is Members: Validate Password now.

[nitriques]

Yes @michael-e please send a PR 👍

[nitriques]

@michael-e would you consider #44 to be fixed when this is merged ?

[michael-e]

Yes and no. Let me explain.

Issue #44 is about logging a Member in after registration. But in the corresponding discussion, other points were raised, such as auto-activating a Member after registration (and at the same time changing the Member role based on POST values). These are completely different requirements IMHO. Off-topic.

So let's talk about logging a Member in after registration exclusively.

Yes, it is possible. But unfortunately, the member-login-info pseudo-event uses the FrontendProcessEvents delegate, which is triggered before real events are executed. So in our case the "logged-in" status will not be shown in the pseudo-event's XML, and so the frontend can not reflect the status correctly.

To solve this, you might add a redirect to the registration form. The "next page" will show the correct login status, of course. If we say that this is a proper solution, we could simply add this use case (describing the redirect necessity) to the README file.

There is another solution as well: If we change the member-login-info pseudo event to use the FrontendEventPostProcess delegate, it will be triggered after all real events. This is possible because the pseudo event only needs the wrapper element from the context (which is passed by reference in both delegates — it's $context['wrapper'] in the first delegate, $context['xml'] in the second one). However, changing the delegate might be considered a breaking change.

Here's an illustration how these delegates and events work (from top to bottom):

Although this would be a breaking change (requiring a 2.0 release), I prefer it. Conceptionally, it feels much better to me to check the login status after all events and their filters have been executed.

What do you think?

[nitriques]

Thanks @michael-e for this explanation.

after all events and their filters have been executed.

Processing the login AFTER events seems counter intuitive to me. Would not this make it impossible to use the 'Can create new' feature on the same page load ? Would this be ok ?

What about creating a REAL event for the login phase ?

[michael-e]

Processing the login AFTER events seems counter intuitive to me.

Maybe you are right. And yes, this might break stuff.

What about creating a REAL event for the login phase ?

What so you mean? An additional event?

Maybe the best solution for now would be my first suggestion — just add some explanation to the README. (Auto-login after registration is not a typical use case IMHO, and at least it can be done, using a redirect.)

[michael-e]

If you agree that we should just add this rare use case to the README, I will care for it.

[nitriques]

What about creating a REAL event for the login phase ?

What so you mean? An additional event?

Yeah!!

If you agree that we should just add this rare use case to the README, I will care for it.

Yes please go ahead, send a PR.

[michael-e]

What about creating a REAL event for the login phase ?

Hmmm, that would make things really hard to understand. I assume that we'd keep the pseudo event (for backwards compatibility) and add another "real" Members event that does the same (again), just a bit later (when other events are resp. have been executed). Too much confusion IMHO.

Yes please go ahead, send a PR.

Will do.

[michael-e]

I added to the existing PR #304. Should be fine now.

[nitriques]

Thanks @michael-e

Too much confusion IMHO.

When I first install the extension, I was searching for the login event and could not find it. It felt weird that the login was not an event.

[michael-e]

It felt weird that the login was not an event.

I agree, but switching from the current "pseudo event" to a real event would be a breaking change, simply because this would potentially change execution order. It would be hard to say "it's executed first", even with high event priority. (Events with the same priority will be executed in alphabetical order, AFAIK.) I guess this is why @brendo used a pseudo event hooked to a delegate — it's predictable.

[nitriques]

to a real event would be a breaking change

Ah yeah it would be a pain.

it's predictable.

It NEEDS to stay that way also ;) I was not suggesting to do it right now, but when possible.

[michael-e]

@nitriques: My PR has been merged and released, so shall we close this?

[nitriques]

@michael-e Yes thanks!