-
Notifications
You must be signed in to change notification settings - Fork 1
Invalidate dependent elements #37
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
base: main
Are you sure you want to change the base?
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
9dfee3b to
cfd6107
Compare
7fc57dd to
6aedd15
Compare
|
|
||
| $this->invalidateElement($element); | ||
|
|
||
| if (ElementType::Object === ElementType::tryFrom($element->getType())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why only for objects?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn’t make sense in every case.
If an object is required by a document, why should all sites that use this object be invalidated?
If a snippet is required by a page, why should all pages containing that snippet be invalidated?
I think we need to carefully consider in which cases this behavior actually makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It wouldn’t make sense in every case.
You don't know that. Only the app will know, right?
If an object is required by a document, why should all sites that use this object be invalidated?
Probably because it is somehow used in the document, so when it's not invalidated it displays outdated data?
If a snippet is required by a page, why should all pages containing that snippet be invalidated?
Same reason: why should the page require the snippet if it doesn't use it? And if it does, and it isn't invalidated, it shows outdated data.
I think we need to carefully consider in which cases this behavior actually makes sense.
Here I'm with you. Maybe we need an (easy) mechanism that controls whether the dependencies should be invalidated or not (also: which ones?) 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right. The examples were not selected carefully.
Indeed, it should be: a Document is required by an Object. That may or may not make sense, but it’s a case the user of the bundle could have control over.
Also, a Page being required by a Snippet doesn’t sound like a standard case. If we want to do that, it should be considered more carefully.
So the idea was: cases where an Object is required by another Object or by a Document are clear. The other cases are not standard and should not be included in our default behavior.
The best way to handle them is, as you said, through configuration.
48fa75d to
ea6878b
Compare
ea6878b to
af41501
Compare
| $element = match (ElementType::tryFrom($required['type'])) { | ||
| ElementType::Object => $this->elementRepository->findObject($required['id']), | ||
| ElementType::Document => $this->elementRepository->findDocument($required['id']), | ||
| ElementType::Asset => $this->elementRepository->findAsset($required['id']), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to check if invalidating an asset makes sense.
No description provided.