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

BUG: Incorrect Image Caching #4707

Closed
1 task done
so-grimm opened this issue Nov 6, 2023 · 12 comments · Fixed by #4788
Closed
1 task done

BUG: Incorrect Image Caching #4707

so-grimm opened this issue Nov 6, 2023 · 12 comments · Fixed by #4788
Assignees

Comments

@so-grimm
Copy link
Contributor

so-grimm commented Nov 6, 2023

Is there an existing issue for this?

  • I have searched the existing issues

Current Behavior

If you have an image component that displays the metadata (caption, copyright notice, etc.) of an image and then the metadata is changed in the media module, these changes aren't shown in the frontend until the cache is flushed.

Expected Behavior

When changing the metadata of an image, those changes should show up immediately in frontend.

Steps To Reproduce

  1. build an image component with an image caption
  2. fill the image caption with ${this.image.caption}
  3. add module to a page, fill it with an image that has a caption
  4. go to the media module and change the caption of the image
  5. go back to your page -> the caption will still have the old text

Environment

- Flow: 9.0
- Neos: 9.0
- PHP: 8.2

Anything else?

I tested the behavior on Neos 8 as well and it's working correctly there.

@so-grimm so-grimm added the Bug label Nov 6, 2023
@mhsdesign mhsdesign added the 9.0 label Nov 6, 2023
@mficzel
Copy link
Member

mficzel commented Nov 6, 2023

Is this really different in Neos 8.3? I would suspect this data beeing cached with the html from the Fusion Cache.

Maybe we should add something like ${Neos.Caching.resourceTag()} to ensure caches are flushed when resources change. AFAIK we do not have something like that in anywhere.

@so-grimm
Copy link
Contributor Author

so-grimm commented Nov 6, 2023

Yes, it is. I tested it on Neos 8.3 and 7.3 and in both cases the caption changes immediately, but in Neos 9.0 I need to flush the cache to get the new value.

@bwaidelich
Copy link
Member

@mficzel I think we do:

$dispatcher->connect(
AssetService::class,
'assetUpdated',
ContentCacheFlusher::class,
'registerAssetChange',
false
);

@kitsunet
Copy link
Member

kitsunet commented Nov 6, 2023

Yep that! and there is some hidden magic in the uri converter implementation where we convert the asset:// to an actual asset and append a tag to the rendering

@bwaidelich
Copy link
Member

I guess the code from above still stems from a time where we had a "global content cache"..
But with the AssetUsage projection in place it should be possible to re-wire this somehow

@kitsunet
Copy link
Member

kitsunet commented Nov 6, 2023

had a "global content cache"..

huh? what do we have now?

@bwaidelich
Copy link
Member

what do we have now?

Well it's still global, but it depends on the Content Repository.
@dlubitz didn't we have a similar topic (or was it the same) were we thought about some configuration option to opt into "asset usage management"?

@mficzel
Copy link
Member

mficzel commented Nov 6, 2023

Hmmm ... interesting did not know that.

Currently the inner working of registerAssetChange as simply commented out on Neos 9 with TODO: re-implement this based on the code below

Not sure i really like this. We could still decide to add AssetTags like NodeTags explicitly.

  entryTags {
     1 = ${Neos.Caching.assetTag(asset)}
  }

@dlubitz
Copy link
Contributor

dlubitz commented Nov 6, 2023

TBH I can't remember.

AFAIS, nothing has changed here, as the AssetRepository still emit the signal.

@dlubitz
Copy link
Contributor

dlubitz commented Nov 6, 2023

Good catch @mficzel ... 🙈

@kitsunet
Copy link
Member

kitsunet commented Nov 6, 2023

This is interesting, that means we need to flush asset changes on every repository I guess? given that they are global and we can't really know which repository uses it

@dlubitz
Copy link
Contributor

dlubitz commented Dec 1, 2023

@kitsunet That's covered internally as the AssetUsage iterates over all CRs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants