Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Undefined variable in branch 3.x #8868

Closed
lindesbs opened this issue Mar 22, 2018 · 15 comments
Closed

Undefined variable in branch 3.x #8868

lindesbs opened this issue Mar 22, 2018 · 15 comments
Labels
Milestone

Comments

@lindesbs
Copy link

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

@lindesbs lindesbs changed the title Undefined variable in branch 3.5 Undefined variable in branch 3.x Mar 22, 2018
@leofeyer leofeyer added this to the 3.5.35 milestone Mar 22, 2018
@leofeyer
Copy link
Member

leofeyer commented Mar 22, 2018

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?

@lindesbs
Copy link
Author

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

@leofeyer
Copy link
Member

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

@lindesbs
Copy link
Author

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

@aschempp
Copy link
Member

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...

@aschempp
Copy link
Member

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.

@lindesbs
Copy link
Author

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

@leofeyer
Copy link
Member

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?

@leofeyer leofeyer modified the milestones: 3.5.35, 3.5.36 Apr 18, 2018
@aschempp
Copy link
Member

aschempp commented Apr 18, 2018

@leofeyer
Copy link
Member

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

@lindesbs
Copy link
Author

Not yet tested

@leofeyer
Copy link
Member

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

@lindesbs
Copy link
Author

Obviously no further impact

@leofeyer
Copy link
Member

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

@leofeyer
Copy link
Member

leofeyer commented Sep 7, 2018

Fixed in 148cfde.

@leofeyer leofeyer closed this as completed Sep 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants