Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[V5] Cache loader improvements #1912

Merged
merged 2 commits into from
May 5, 2022
Merged

[V5] Cache loader improvements #1912

merged 2 commits into from
May 5, 2022

Conversation

erikn69
Copy link
Contributor

@erikn69 erikn69 commented Nov 4, 2021

  • Deleted old V4 fallbacks
  • Early return used
  • Avoid use same bucle twice on permisions (collect($this->permissions)->map() and later ->each())
  • array_map instead of collect($this->permissions)->map()->all() and other collect() again with the first collect

@erikn69 erikn69 force-pushed the fix_loader branch 2 times, most recently from 78ecd2f to f224b25 Compare November 4, 2021 17:41
@freekmurze
Copy link
Member

To me, the old and new code is unreadable. Could you split this up in dedicated functions that are clearly named and explain what is happening?

@erikn69 erikn69 force-pushed the fix_loader branch 8 times, most recently from f532174 to e566d2a Compare November 18, 2021 22:05
@erikn69
Copy link
Contributor Author

erikn69 commented Nov 18, 2021

I have another commit that makes cache 50% smaller by serialization of Role model ids,
but I have not uploaded it because I know you like the changes in parts, so I waiting to see if this is ok

TamanoCache

@freekmurze
Copy link
Member

Will these cache improvements break if people have already cached things using the old way?

@erikn69
Copy link
Contributor Author

erikn69 commented Dec 17, 2021

@freekmurze no breaks, there is a if that forget cache if it is old versions, a temporal fix for that

// must be removed on next mayor version
 if (! isset($this->permissions['permissions'])) {
    $this->forgetCachedPermissions();
    $this->loadPermissions();

    return;
}

Copy link

@Restingo Restingo left a comment

Choose a reason for hiding this comment

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

Typo

src/PermissionRegistrar.php Outdated Show resolved Hide resolved
src/PermissionRegistrar.php Outdated Show resolved Hide resolved
@angeljqv
Copy link
Contributor

angeljqv commented Apr 5, 2022

@freekmurze will this PR be merged?

@freekmurze freekmurze merged commit 4e3ad0a into spatie:main May 5, 2022
@freekmurze
Copy link
Member

Thanks! 👍

@erikn69
Copy link
Contributor Author

erikn69 commented May 5, 2022

Thank you, i am getting better times and less ram usage
image

@erikn69 erikn69 deleted the fix_loader branch February 17, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants