Skip to content

remove unnecessary group_types array and associated methods. - #606

Open
nickchomey wants to merge 3 commits into
rhubarbgroup:developfrom
nickchomey:remove-group-types
Open

remove unnecessary group_types array and associated methods.#606
nickchomey wants to merge 3 commits into
rhubarbgroup:developfrom
nickchomey:remove-group-types

Conversation

@nickchomey

Copy link
Copy Markdown
Contributor

the performance optimizations introduced with the associative array in #340 didnt actually require adding the group_types array and associated methods. Better to just convert global/ignored/unflushable_groups themselves to associative arrays and use isset on those directly.

…gnored/unflushable_groups are now associative arrays
Copilot AI review requested due to automatic review settings March 26, 2026 12:52

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR simplifies group-type handling in the object cache by removing the derived group_type mapping and relying on direct group membership checks (via associative arrays + isset()), continuing the performance-oriented direction of #340.

Changes:

  • Convert global_groups, ignored_groups, and unflushable_groups into associative lookup structures and use isset() for membership checks.
  • Remove cache_group_types() / is_group_of_type() and update callers to use direct group membership helpers.
  • Adjust selective flush logic to account for the new structure (e.g., array_keys() for unflushable groups).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread includes/object-cache.php Outdated
Comment thread includes/object-cache.php
Comment thread includes/object-cache.php Outdated
Comment thread includes/object-cache.php Outdated
Comment thread includes/object-cache.php
Comment thread includes/object-cache.php
Comment thread includes/object-cache.php
Comment thread includes/object-cache.php

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread includes/object-cache.php
Comment thread includes/object-cache.php
Comment thread includes/diagnostics.php
Comment thread includes/diagnostics.php Outdated
Comment thread tests/Feature/CacheTest.php Outdated
Comment thread includes/diagnostics.php Outdated
@nickchomey

Copy link
Copy Markdown
Contributor Author

i admittedly did not test the new test, nor whether the old ones passed - not sure how to actually run them. Do you have any CI tests that do it? Or are you able to run yourself easily?

@nickchomey

Copy link
Copy Markdown
Contributor Author

@tillkruss were you able to look at the tests for this?

@tillkruss

Copy link
Copy Markdown
Member

@nickchomey I was under the impression that we're waiting on something in SQL object cache to be figured out first, is that one merged?

@nickchomey

Copy link
Copy Markdown
Contributor Author

It hasn't been merged in sqlite object cache either. But it's not clear to me how that is relevant? I'm happy to address anything that remains of concern here

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.

3 participants