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

Add support for undated collections #4

Merged
merged 7 commits into from Mar 4, 2024

Conversation

aerni
Copy link
Contributor

@aerni aerni commented Mar 1, 2024

This PR adds the ability to invalidate entries of any collection (not just dated) based on provided query scopes. This opens up countless possibilities to invalidate entries based on other fields than the date.

I also took the chance to make the query scopes config more powerful by allowing granular scopes.

How it works

  • Undated collections are only queried once a query scope has been defined in the config. If there are no custom query scopes, no entries of undated collections are queried nor invalidated.
  • Dated collections always have the Now query scope applied in addition to any custom query scope.

Examples

Apply a scope to all collections.

'query_scopes' => \Path\To\Scope::class,

Apply multiple scopes to all collections:

'query_scopes' => [
    \Path\To\Scope::class,
    \Path\To\DifferentScope::class,
],

Apply scopes to specific collections:

'query_scopes' => [
    'collection_handle' => \Path\To\Scope::class,
    'another_collection' => \Path\To\DifferentScope::class,
],

Apply multiple scopes to specific collections:

'query_scopes' => [
    'collection_handle' => [
        \Path\To\Scope::class,
        \Path\To\DifferentScope::class,
    ],
    'another_collection' => [
        \Path\To\Scope::class,
        \Path\To\DifferentScope::class,
    ],
],

Disclaimer

This PR is technically a breaking change in the case of a user applying a global query scope without specifying the collection. (As in config option 1 and 2). After this PR, global query scopes will be applied to dated AND undated collections. So this could result in undesired invalidation of undated entries.

There are a couple of ways we could handle this:

  • We could ignore undated collections when query scopes have been configured globally
  • We could add a feature flag, so the user can opt into undated collections
  • We could ignore this breaking change and YOLO
  • Or we release a new major version

@aerni aerni changed the title Feature/any collection Add support for undated collections Mar 1, 2024
@martyf
Copy link
Contributor

martyf commented Mar 3, 2024

Thanks for this @aerni. Regarding the disclaimer, I think points 1, 2 or 4 are valid options.

The issue with 4 is people may miss it - but it is also the cleanest way to say "forget the past, just maintain that base going forward" which I like.

From another person's perspective, what would you like to see as a user?

@aerni
Copy link
Contributor Author

aerni commented Mar 3, 2024

I don't mind a major release with a breaking change. Especially in a situation like this, where it's a fundamental change upon which new features will be built. A feature flag feels a little complicated.

One thing to consider before releasing a major version is if this is really the best way to do things before moving forward. There might be some other features that would be nice to implement, but which might not work with the current state of this PR either. Resulting in a potential other major release sooner than later. So a major release should be thoughtful.

@martyf
Copy link
Contributor

martyf commented Mar 3, 2024

Did you have thoughts or suggestions in mind, whether for now or for later?

The whole concept is really opinionated to start with so definition of "best way" comes with that caveat anyway ;)

@aerni
Copy link
Contributor Author

aerni commented Mar 3, 2024

One feature that would be handy is the ability to invalidate an entry based on a boolean. This is particularly useful in combination with computed properties.

Collection::computed('events', 'is_live', fn ($entry) => $entry->is_digital_event ? Carbon::parse($entry->date)->subMinutes(15) < now() : null);

So instead of having to duplicate the logic of the computed property in a query scope, we can just query by is_live and validate the entry when it returns true.

You can already query by is_live but the entry would be invalidated every time the command is run as the entry would be queried every time. So we'd need a way to only invalidate it the first time is_live returns true.

This is just icing on the cake, though.

@martyf
Copy link
Contributor

martyf commented Mar 4, 2024

But would you see that a v2 extension to this, or something that is needed before considering a v2?

@aerni
Copy link
Contributor Author

aerni commented Mar 4, 2024

Depends. I don't think this is needed for v2. But to implement this feature, we might have to refactor and introduce another breaking change, hence v3. So if this is a feature that you'd consider, it might be best to first dig into it to see if it's doable with the current code base before releasing v2.

@martyf
Copy link
Contributor

martyf commented Mar 4, 2024

I'm not sure I have a use case for it personally (and quite have my head around what you've described) - but if it is something you wanted to look at because it would help with how you could use the addon, more than happy to include it. The initial purpose was really just to flush based on dates - and I love how this has grown with query scopes and the work you've done, so can see potential in future expansions for sure.

@aerni
Copy link
Contributor Author

aerni commented Mar 4, 2024

I might look at implementing this feature in the future. For now, I'm fine with the state of this PR. I'd release v2. If anything breaks in the future, we can always release v3.

@martyf martyf merged commit e825ce2 into mitydigital:main Mar 4, 2024
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

2 participants