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

Access permissions 5: DRY permission caching #6881

Open
wants to merge 1 commit into
base: v5/feature/access-permissions-4
Choose a base branch
from

Conversation

lukasbestle
Copy link
Member

@lukasbestle lukasbestle commented Dec 23, 2024

🚨 Merge first

Description

Summary of changes

Move caching of access and list permissions to ModelPermissions

Reasoning

DRY code, especially now that we would add five more of these methods to the User, Site and Language classes

Additional context

We could cache all other permissions as well, but I've wrestled with unit tests for too long yesterday and decided to go with this intermediate solution for now.

The new UserPermissions::cacheKey() is currently uncovered as the User class does not have access/list permissions at the moment. This will be fixed in the PR 7.

Changelog

Refactoring

  • Globally cache access and list permissions per permission category, model type and user role to reduce code duplication

Breaking changes

None

Docs

None

Ready?

  • In-code documentation (wherever needed)
  • Unit tests for fixed bug/feature
  • Tests and CI checks all pass

For review team

  • Add lab and/or sandbox examples (wherever helpful)
  • Add changes & docs to release notes draft in Notion

@lukasbestle lukasbestle added the type: refactoring ♻️ Is about bad code; cleans up code label Dec 23, 2024
@lukasbestle lukasbestle added this to the 5.0.0-beta.2 milestone Dec 23, 2024
@lukasbestle lukasbestle requested a review from a team December 23, 2024 10:10
@lukasbestle lukasbestle self-assigned this Dec 23, 2024
@lukasbestle
Copy link
Member Author

I think this PR really does have a performance impact, see #6880 (comment). With these changes, we need to create a ModelPermissions instance for each model and not just for each combination of template and user role.

I wonder if there is a better way to DRY the logic without this performance regression. Or otherwise we should probably discard this and add the same caching logic to the new isAccessible()/isListable() methods for site/user/language in upcoming PR 7.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: refactoring ♻️ Is about bad code; cleans up code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant