Skip to content

Conversation

@ncla
Copy link
Contributor

@ncla ncla commented Oct 10, 2022

Fixes #4595

The fix in the issue description works fine. It also affects focus being saved as it goes under data property too.

I thought that generateMeta method in Asset class already accounts for existing asset data and merges it together here:

https://github.com/ncla/cms/blob/3ed1e592b309f34379db07fb95af406b97fe87f8/src/Assets/Asset.php#L235-L253

But with line $meta = ['data' => $this->data->all()];, as far as I understood it is for use cases where you are creating Asset object manually? So as that is not the case here, we have to simply hydrate the asset, as AssetRepository::save() does not account for such situation.

It is an additional call to file system for generating meta. No matter how I look at it, the command will have to look up the meta file, unless we can pull this data from stache/cache or something?

I think most people are fine with this behavior (?) so there's no need to include a separate command flag like --without-hydration for disregarding existing data, or to make the command create less file system calls. I am just wondering as there's lately been some issues with the asset hydration, so just want to be considerate.

As bonus I have added tests to sleep better at night. Borrowed, inspired logic from other asset tests.

@jasonvarga jasonvarga merged commit 7ce9b4d into statamic:3.3 Nov 1, 2022
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.

php please assets:meta cleanup data in yaml meta files

2 participants