Lychee icon indicating copy to clipboard operation
Lychee copied to clipboard

Fixes #1506

Open nagmat84 opened this issue 3 years ago • 5 comments

Fixes #1506.

Occasionally, updating Lychee fails because users explicitly cached the Lychee/Laravel configuration, forget about it and after a package update strange things start to happen, because the Laravel caches still contain outdated configurations or source files.

This PR prevent this from happening by running artisan optimize:clear during update.

Actually, I am not 100% pleased with this PR. For me artisan optimize has an observable impact on response time on my production setup. Hence, I feel a little bit reluctant about running artisan optimize:clear without also running artisan optimize after the update to re-create the cache again. Note, that the problematic cache is only built in the first place if users deliberately had run artisan optimize in the past. Hence, this PR defies the explicit will of a user. Optimally, we would run artisan optimize for production environments and not run it for development environments. But there seems to be now way how to run Composer scripts based on the environment. There is only a -dev section for package dependencies, but not for scripts.

nagmat84 avatar Sep 10 '22 10:09 nagmat84

Codecov Report

Merging #1516 (29d2c8f) into master (fd445f6) will decrease coverage by 0.95%. The diff coverage is n/a.

:exclamation: Current head 29d2c8f differs from pull request most recent head b2bd466. Consider uploading reports for the commit b2bd466 to get more accurate results

Additional details and impacted files

codecov[bot] avatar Sep 10 '22 11:09 codecov[bot]

You could check the existence of vendor/bin/phpunit or vendor/bin/phpstan to determine if dev is being used or prod. :)

ildyria avatar Sep 10 '22 18:09 ildyria

I know, I could, but I actually don't feel like doing it. What I meant is, there is no option to do it inside Composer using the means offered by Composer.

First of all, it would require just another script and I feel like we already have too many scripts (in too many different script languages) and I don't want to add another one onto that pile. Secondly, using the existence of a particular file which "accidentally" happens to exist as a development dependency as an indicator whether Composer is in development mode or not feels kind of nasty workaround. In particular, it feels like something which might break at some point and then nobody knows why.

So, yes I wanted to fix #1506 with as little complications as possible. I don't mind if this PR gets dropped and #1506 stays open or gets closed as "won't fix". In particular, as the users brought themselves into that situation in the first place. I also don't mind if someone extends this PR and fixes #1506 in a better way.

nagmat84 avatar Sep 10 '22 21:09 nagmat84

At least the post-merge is a script already. Does php artisan optimize create some files with predictable names? We could run optimize:clear followed by optimize only if the files in question already exist...

kamil4 avatar Sep 11 '22 22:09 kamil4

Actually, opimize is a meta command to cache the routes, the configuration and the views. We could probably check the existence of

inside our own console command. Checking for compiled views seems to a little bit more complicated as there does not seem to be a single file, but at least one file for each compiled view.

nagmat84 avatar Sep 12 '22 16:09 nagmat84

@ildyria and @kamil4: You have already approved the PR but I would like to ask you to re-review it again as it has changed significantly. As suggested by @kamil4 in https://github.com/LycheeOrg/Lychee/pull/1516#issuecomment-1243054247 and discussed in https://github.com/LycheeOrg/Lychee/pull/1516#issuecomment-1244006596 I decided to improve the original Laravel command optimize.

nagmat84 avatar Sep 27 '22 16:09 nagmat84