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

[v4] Plugin assets resolves as JSON (500 Internal Server Error) #5836

Closed
afbora opened this issue Oct 20, 2023 · 9 comments · Fixed by #5891
Closed

[v4] Plugin assets resolves as JSON (500 Internal Server Error) #5836

afbora opened this issue Oct 20, 2023 · 9 comments · Fixed by #5891
Assignees
Milestone

Comments

@afbora
Copy link
Member

afbora commented Oct 20, 2023

Description

Sometimes plugin assets resolves as JSON. This issue does not always occur. I don't know exactly when it occurs.

Request URL: /media/plugins/afbora/test/2538303991-1697822431/css/style.css
Request Method: GET
Status Code: 500 Internal Server Error
Content-type: application/json; charset=UTF-8

Refused to apply style from '/media/plugins/afbora/test/2538303991-1697822431/css/style.css' because its MIME type ('application/json') is not a supported stylesheet MIME type, and strict MIME checking is enabled.

Use following code to reproduce:

<?= css(kirby()->plugin('afbora/test')->asset('css/style.css')) ?>

To reproduce

  1. Download plainkit with v4 here: plugin-assets-resolve.zip
  2. Go to homepage and open browser console
  3. Try to refresh the page multiple times
  4. Check browser console after each refresh
  5. You'll see 500 internal server issue for a asset

Your setup

Kirby 4.0.0-beta.3/v4-develop

Additional context

When I checked /media/plugins/afbora/test directory, It's empty. When I visit /media/plugins/afbora/test/2538303991-1697822431/css/style.css url from browser, opens correctly in browser and I can see the style.css in /media/plugins/afbora/test. But I refresh the homepage, /media/plugins/afbora/test directory cleared again.

@distantnative
Copy link
Member

Sounds like something in PluginAssets::clear() removing files that are actually still in use and not stale

@afbora
Copy link
Member Author

afbora commented Oct 20, 2023

Seems PluginAssets::resolve() always clean before process: https://github.com/getkirby/kirby/blob/v4/develop/src/Cms/PluginAssets.php#L140-L141

@distantnative
Copy link
Member

That is right, that's the only time it can run. But ::clean() must not match still active files. So I'd think the bug is there

@afbora afbora added this to the 4.0 milestone Oct 20, 2023
@afbora
Copy link
Member Author

afbora commented Oct 24, 2023

@distantnative Looks like works like following. Not sure but I think bug is related with array_diff arguments order. Do you think it could be that easy? 🙂

- $stale = array_diff($files, $active);
+ $stale = array_diff($active, $files);

https://github.com/getkirby/kirby/blob/v4/develop/src/Cms/PluginAssets.php#L49

Update: Nvm, I think this is not related with the issue.

@afbora
Copy link
Member Author

afbora commented Oct 24, 2023

Another clue is about getting current files via Dir::index(). This returns dirs and files. But $active files only includes files. So array_diff compares wrong lists. I think it should be like that:

- $files = Dir::index($media, true);
+ $files = Dir::files($media);

https://github.com/getkirby/kirby/blob/v4/develop/src/Cms/PluginAssets.php#L37

@distantnative
Copy link
Member

Yes that's part of the problem. Another part is that actually with our new asset plugin extension the names might not even match. So we would probably also add calls like $plugin->asset($file) to really match names. And then the question remains how to clean up empty folders in media.

@lukasbestle
Copy link
Member

I think the main issue right now is indeed that the folders are only included in the current index, not the $active list. This makes them always appear in $stale, so I guess all subdirectories are always deleted.

We could fix this by including all parent directories up to the plugin's media root for all assets in $active as well. Then array_diff() will not put them in $stale anymore.

@distantnative
Copy link
Member

That's but also assets/css/style.css will be just as css/style.css in the media folder. And if we register an asset with a custom key as path that doesn't match the actual filesystem
path, it can even be that css/style.css in the media folder would belong to framework/styling/index.css in the plugin folder

@lukasbestle
Copy link
Member

Yeah, we definitely also need to look up the resulting paths for each asset.

distantnative added a commit that referenced this issue Oct 29, 2023
@distantnative distantnative modified the milestones: 4.0, 4.0-beta.3 Oct 29, 2023
@distantnative distantnative linked a pull request Oct 29, 2023 that will close this issue
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

4 participants