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

Fixes #1506 #1516

Merged
merged 8 commits into from
Sep 28, 2022
Merged

Fixes #1506 #1516

merged 8 commits into from
Sep 28, 2022

Conversation

nagmat84
Copy link
Collaborator

@nagmat84 nagmat84 commented Sep 10, 2022

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 --clever --dont-ask=assume-no during update.

This PR also puts the original optimize command provided by the framework on steroids. The extended optimize command allows to only build a cache if the user has explicitly chosen to do so in the past. From the comment above the new command:

/**
 * Improves the original "optimize" command provided by the framework.
 *
 * There are three improvements:
 *
 *  1) This command explicitly clears the old cache before building a new one.
 *     Note that rebuilding a new cache is not sufficient, because this
 *     does *not* overwrite the entire cache but some leftovers may remain.
 *     This actually looks like an oversight and bug in the original command.
 *  2) This command adds a "clever" mode which only rebuilds a new cache if
 *     a previous cache has existed.
 *     We use this in our install/update scripts in order to rebuild the cache
 *     after installation/update without enforcing to use a cache for everyone.
 *  3) This command adds a confirmation, if the user requests to build a cache
 *     for non-productive environments as this is most likely an error and
 *     undesired.
 *     The confirmation can be skipped by pre-selecting the answer via a
 *     command line option.
 */

@nagmat84 nagmat84 requested a review from a team September 10, 2022 10:57
@codecov
Copy link

codecov bot commented Sep 10, 2022

Codecov Report

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

❗ 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

@ildyria
Copy link
Member

ildyria commented Sep 10, 2022

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

@nagmat84
Copy link
Collaborator Author

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.

Copy link
Contributor

@kamil4 kamil4 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (untested).

@kamil4
Copy link
Contributor

kamil4 commented Sep 11, 2022

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...

@nagmat84
Copy link
Collaborator Author

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 nagmat84 marked this pull request as draft September 26, 2022 19:15
@nagmat84 nagmat84 marked this pull request as ready for review September 27, 2022 16:22
@nagmat84
Copy link
Collaborator Author

@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 #1516 (comment) and discussed in #1516 (comment) I decided to improve the original Laravel command optimize.

Copy link
Member

@ildyria ildyria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

app/Console/Commands/Optimize.php Outdated Show resolved Hide resolved
app/Console/Commands/Optimize.php Outdated Show resolved Hide resolved
app/Console/Commands/Optimize.php Show resolved Hide resolved
app/Console/Commands/Optimize.php Show resolved Hide resolved
composer.json Show resolved Hide resolved
scripts/post-merge Show resolved Hide resolved
nagmat84 and others added 2 commits September 28, 2022 17:15
Co-authored-by: Kamil Iskra <kamil.01482@iskra.name>
@nagmat84 nagmat84 merged commit f905e22 into master Sep 28, 2022
@nagmat84 nagmat84 deleted the fixes-1506 branch September 28, 2022 16:18
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.

500 HttpException while upgrading Lychee from 4.5.3 to 4.6.0
3 participants