Improve event filters #304
Conversation
| @@ -703,7 +703,7 @@ public function appendFilter($context) { | |||
| ); | |||
| } | |||
|
|
|||
| if(!FieldManager::isFieldUsed(self::getFieldType('authentication'))) { | |||
| if(FieldManager::isFieldUsed(self::getFieldType('authentication'))) { | |||
There was a problem hiding this comment.
This does not make it show "always", but only if a authentication field is used. It was a bug ?
There was a problem hiding this comment.
Good catch! Yes, I can only suspect that this was a bug, introduced by accident. The filter will call the filter_UpdatePasswordLogin function, which is completely useless without an authentication field. So the filter itself has been useless all the time, and I don't consider this a breaking change but a bugfix. (Please, confirm! I don't want to break stuff, of course.)
For the same reason showing the filter is only useful if an authentication field exists, so IMHO "always if the field exists" is enough.
There was a problem hiding this comment.
BTW, I found the misunderstanding. In #44 (comment) @jensscherbl said:
Added this as a filter (Members: Login), which is only available if no activation is needed. Just like @brendo suggested.
So the idea was to only show the filter if no activation field exists. But actually he tested for the authentication field. I don't know why nobody ever noticed this.
(I don't think that we need to test for the activation field. It's up to the developer to use the the filter (and see if it works). The filter just attempts to do what the name says: Log the Member in.)
| return; | ||
| } | ||
|
|
||
| if (is_null($this->section->getFieldHandle('authentication'))) { |
There was a problem hiding this comment.
We could do ! instead of is_null ?
There was a problem hiding this comment.
By design getFieldHandle will return null or a string, so why do you prefer !?
I'd prefer to be consistent with the rest of the code. If you search the extension for $this->section->getFieldHandle, you will see that is_null is always used to test the return value.
There was a problem hiding this comment.
So if we need to change how these return values are tested, I suggest to open another issue.
lib/member.symphony.php
Outdated
|
|
||
| $member_id = null; | ||
| if (isset($_POST['fields'][$this->section->getFieldHandle('authentication')]['validate'])) { | ||
| $password = Symphony::Database()->cleanValue( |
There was a problem hiding this comment.
We do not need to clean the password, as it will be hashed anyways.
|
I will wait a bit with merging, because:
|
This commit adds information about the "immediate login after registration" procedure and some hints about the (execution) order of Member info and events/filters.
|
IMHO this can be pulled now. #303 can be closed then. I suggest to release 1.10.0, because we have new features. |
This PR improves event filters as described in #303.
Additionally, the last commit (02aac90) adds additional constants to the
MemberEventMessagesclass and implements error codes for all Event Filters. Please note that adding these error codes to Filters (as opposed to Events) might be controversial, so I am open to feedback!