DarkaOnLine / L5-Swagger

OpenApi or Swagger integration to Laravel
https://github.com/DarkaOnLine/L5-Swagger
MIT License
2.64k stars 393 forks source link

Properly publish package assets/resources #112

Closed jradtilbrook closed 6 years ago

jradtilbrook commented 6 years ago

Description:

From reading the official Laravel documentation and using this package, I think it is not correctly and fully publishing the resources included. The helper functions that take in assets and return the full path so a controller can return them are not working for me (see below). I think, since these are static files, it would be better if it didn't go through a controller anyway and was just served from the public directory. This would result in multiple benefits:

I believe the change is minimal; probably less than 10 line changes and would result in a much more modular package. Would you consider these changes? I'm happy to make a PR

At the same time, 3 of the 4 commands included in this package are redundant and provided by Laravel already (if this new approach is taken). I propose removing those commands to deduplicate and remove extra complexity that needs testing/verification - who doesn't like removing code :wink:

Steps To Reproduce:

Half of the issue is my current use case of installing Laravel to a subdirectory of an existing project where the vendor directory is found in another location (not under the root of Laravel). However, this can be resolved if the assets are properly published, allowing override.

jradtilbrook commented 6 years ago

I only just realised that the static assets aren't part of this package so I guess this makes this issue almost completely redundant! :face_with_head_bandage:

DarkaOnLine commented 6 years ago

@jradtilbrook thank's for your insights.

Regarding assets, like you notices they are not part of the package. Till version 5 we were managing assets by hand. And it was pain in the ass :) Our assets almost every time was outdated :(

Regarding publish commands, it would be a great improvement, please make a PR!

DarkaOnLine commented 6 years ago

By the way, you always can publish view file and change it as you want. You can download swaager-ui assets for your project and load them statically.

jradtilbrook commented 6 years ago

Yeah that's what I ended up doing. Sorry for raising this issue without enough research.
Sounds good, I'll make a PR for the publish commands - do you want it attributed to this issue, or should it be closed now since the major part won't be done?

DarkaOnLine commented 6 years ago

You can attribute PR to this issue, it's ok for me. Thanks again !!! 🎉