contao/core

Undefined variable in branch 3.x

Closed this issue · 15 comments

Beginning of branch 3.x the variable blnRecordExists is used without initialized with data. This bug results in cleaning autologin data on logout()

The variable blnRecordExists in branch 2.11
Initialization in
https://github.com/contao/core/blob/2.11/system/libraries/Model.php#L77

Defined when row exists
https://github.com/contao/core/blob/2.11/system/libraries/Model.php#L178

In the branch 3.x it is used in
https://github.com/contao/core/blob/3.5/system/modules/core/classes/FrontendUser.php#L244

But this variable will never be defined

That is true, however I don't see how this can result in cleaning the data? Since the property is never initialized, it should be null and thus result in not cleaning the data. Did you mean this?

Yes. Users which has been self logout manually, will be relogedin automatically.

@contao/developers I guess we can just remove the if condition entirely, can't we?

Our fix is replacing with this:
if ($this->autologin)

Very interesting find. The variable is undefined because the User class is no longer based on Model since Contao 3. I don't understand why the check for blnRecordExists is necessary at all though...

Well it is possible to do a FrontendUser::getInstance() call and call logout, which would be an unexpected result. We should probably check for $this->intId > 0.

There is an additional problem with ModuleCloseAccount. We have to check, if the MemberRow still exists. The entry will be deleted afterwards it will be logout()
https://github.com/contao/core/blob/3.5/system/modules/core/modules/ModuleCloseAccount.php#L135

IMHO, $blnRecordExists should simply be replaced with \Config::get('autologin') > 0 (see FrontendUser::login().

@aschempp What is the case in which we have to check $this->intId > 0?

@lindesbs Does this issue occur in Contao 4 as well?

Not yet tested

@lindesbs Do you have time to check? If it occurs in Contao 4 as well, we have to fix it.

Obviously no further impact

I still think we should fix this if we implement #8888.

Fixed in 148cfde.