Skip to content

Adjust to latest atk4/data#77

Merged
mvorisek merged 45 commits intodevelopfrom
upgrade_develop
Nov 23, 2021
Merged

Adjust to latest atk4/data#77
mvorisek merged 45 commits intodevelopfrom
upgrade_develop

Conversation

@mvorisek
Copy link
Copy Markdown
Member

@mvorisek mvorisek commented Oct 16, 2021 •
edited
Loading

No description provided.

@PhilippGrashoff
Copy link
Copy Markdown
Contributor

PhilippGrashoff commented Oct 17, 2021

Hi there,

I agree that the password field should be moved to atk4/data.
This repository has so much mixed code (User model, Password field, Authentication check, Password suggestions, some UI classes) that to me it seems overloaded. If password field was moved atk4/data I would simply copy the simple Auth code and get rid of the dependency to atk4\login...

@mvorisek mvorisek force-pushed the upgrade_develop branch 3 times, most recently from bbda325 to 89951c9 Compare October 18, 2021 11:42
mvorisek
mvorisek commented Oct 18, 2021
View reviewed changes
src/Auth.php
protected function loadFromCache(): void
{
if (isset($this->cache->getData()[$this->user->id_field])) {
$this->user = $this->user->getModel()->load($this->cache->getData()[$this->user->id_field]);
Copy link
Copy Markdown
Member Author

@mvorisek mvorisek Oct 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@DarkSide666 change in https://github.com/atk4/login/pull/66/files#diff-ffd530ee6eec28dabbc93a431fb04b89cb75a01c93f911e22d46aeeaf17d683eR214 was breaking tests as model was never loaded before and thus complete dirty data were null. Was this change intended and what is the reason to cache the whole User model's data? (as they are not locked in db, they will be always overwritten this way).

The change done now by me is to fix the dirty data calc, but it does not fix the wrong caching described above.

@mvorisek mvorisek force-pushed the upgrade_develop branch 10 times, most recently from e996eb5 to d113b8d Compare October 19, 2021 10:41
@mvorisek mvorisek force-pushed the upgrade_develop branch 4 times, most recently from 9b1cc7d to bbd9b69 Compare November 18, 2021 13:19
@mvorisek mvorisek marked this pull request as ready for review November 18, 2021 13:21
@mvorisek mvorisek force-pushed the upgrade_develop branch 5 times, most recently from 9373f18 to 51c81cb Compare November 19, 2021 00:49
@mvorisek mvorisek mentioned this pull request Nov 19, 2021
Closed
@mvorisek mvorisek merged commit bcc180e into develop Nov 23, 2021
@mvorisek mvorisek deleted the upgrade_develop branch November 23, 2021 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DarkSide666 DarkSide666 DarkSide666 approved these changes

@abbadon1334 abbadon1334 Awaiting requested review from abbadon1334

@georgehristov georgehristov Awaiting requested review from georgehristov

@ibelar ibelar Awaiting requested review from ibelar

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mvorisek @PhilippGrashoff @DarkSide666