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

Migrate install_files scripts to PHP and add more config to .env #1361

Merged
merged 9 commits into from
Jun 7, 2022

Conversation

qwerty287
Copy link
Contributor

@qwerty287 qwerty287 commented Jun 4, 2022

Allow to change some config options via .env and migrate the install_files to PHP (see #1323 (comment)). Also fixes a deprecation.

@codecov
Copy link

codecov bot commented Jun 4, 2022

Codecov Report

Merging #1361 (f540021) into master (68d60df) will decrease coverage by 0.88%.
The diff coverage is n/a.

@qwerty287 qwerty287 requested review from nagmat84 and ildyria and removed request for nagmat84 June 4, 2022 14:11
Copy link
Collaborator

@nagmat84 nagmat84 left a comment

Choose a reason for hiding this comment

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

Overall I am in favor of the PR. I added some remarks and suggestions, but nothing severe.

config/app.php Outdated Show resolved Hide resolved
config/app.php Show resolved Hide resolved
scripts/install_files.php Outdated Show resolved Hide resolved
scripts/install_files.php Outdated Show resolved Hide resolved
scripts/install_files.php Outdated Show resolved Hide resolved
copy('scripts/post-merge', '.git/hooks/post-merge');

echo "\n${ORANGE}To disable the call of composer and migration on pull add$NO_COLOR\n";
echo "${ORANGE}a file named '.NO_AUTO_COMPOSER_MIGRATE' in this directory.$NO_COLOR\n";
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment is for the whole script not just this line. I am not sure, if this script works nicely on Windows. Obviously, nobody has ever being bothered by this question, because the old file was a .sh-script which obviously has never been working with Windows.

But now, after the script has become runnable for Windows we should maybe also ensure that it runs nicely. Here are the two things, I am not sure about:

  1. We use \n as the new line character. PHP offers the constant PHP_EOL which is \n\r on Windows, because Windows needs an additional carriage return. However, I do not know if this is only required for text files our for output to the command line, too. I would suspect the letter, because the programmer cannot know if the STDOUT targets the screen or is redirected to a file. So probably, we should use PHP_EOL.
  2. I am not sure, if Windows can handle the ANSI escape sequence and color codes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not tested, but Windows seems to support ANSI. See https://gist.github.com/mlocati/fdabcaeb8071d5c75a2d51712db24011 for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! Then we can keep the color codes. I feared that Windows would get confused in such a way that it would totally break console output. Glad to hear that it is the other way around!

But I still believe we need PHP_EOL instead of \n.

Moreover, another thought crossed my mind. I somehow remember that you figured out that the ANSI color codes are either only supported by echo or by printf. I don't remember what script you repaired and what the correct solution was, but have you checked that?

qwerty287 and others added 4 commits June 5, 2022 09:10
Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de>
Co-authored-by: Matthias Nagel <matthias.h.nagel@posteo.de>
config/session.php Show resolved Hide resolved
config/hashing.php Outdated Show resolved Hide resolved
Copy link
Contributor

@d7415 d7415 left a comment

Choose a reason for hiding this comment

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

Looks good. I think it's worth adding some of the missing variables to the example .env, so that users know what's available.

config/app.php Show resolved Hide resolved
config/session.php Show resolved Hide resolved
config/trustedproxy.php Show resolved Hide resolved
config/urls.php Show resolved Hide resolved
@qwerty287
Copy link
Contributor Author

I wouldn't add them to .env, instead, use the docs for this task (LycheeOrg/LycheeOrg.github.io#56).

@d7415
Copy link
Contributor

d7415 commented Jun 5, 2022

LycheeOrg/LycheeOrg.github.io#56 (comment)

I'm very in favour of complete examples (ideally with the default values - I suggested changing those to match elsewhere).

As a user, having to copy every directive is a pain, and then you'd probably want to add context in case you need to edit later.

@qwerty287
Copy link
Contributor Author

I understand this and agree with you, but I wouldn't add the experimental/advanced settings to the example. And the non-experimental are all available there already.

@d7415
Copy link
Contributor

d7415 commented Jun 5, 2022

Looking again at the list, I think I mostly agree. At which point it doesn't need to be in this PR.

FWIW, the one that caught my eye was SESSION_SECURE_COOKIE, which seems like a reasonable thing to expose at some point.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 6, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

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.

Untested but looks good.

@qwerty287 qwerty287 merged commit d9fa818 into master Jun 7, 2022
@qwerty287 qwerty287 deleted the php-install branch June 7, 2022 06:07
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.

4 participants