Skip to content

Update totara-docker-dev with support for the new entrypoint#387

Open
angelakuznetsova wants to merge 1 commit intototara:masterfrom
angelakuznetsova:TL-48340
Open

Update totara-docker-dev with support for the new entrypoint#387
angelakuznetsova wants to merge 1 commit intototara:masterfrom
angelakuznetsova:TL-48340

Conversation

@angelakuznetsova
Copy link
Copy Markdown
Contributor

Description

Describe what this pull request is about

Testing Instructions

  1. Check out this pull request
  2. Testing instructions here

Checklist

  • Does what the author says it will do
  • Testing instructions are provided
  • Commit messages make sense and follow the conventional commit standard
  • No identified security issues
  • No identified maintenance issues
  • Any third-party libraries/dependencies use the MIT or Apache 2.0 license
  • Changes made are backwards compatible and will not break existing setups
  • Changes to scripts in the bin/ directory run correctly on both MacOS and WSL
  • Changes to containers can be built locally sucessfully (e.g. via tbuild container && tup container)
  • Containers/images are compatible with both AMD64 (Windows) and ARM64 (MacOS)
  • Changes made to config.php are compatible with our oldest supported Totara version, our newest Totara version, and Moodle

Copy link
Copy Markdown

@NingZhou-NZ NingZhou-NZ left a comment

Choose a reason for hiding this comment

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

Good job! I just had a few suggestions. Have a look when you have a moment. :)

@@ -10,6 +10,7 @@ client_max_body_size 1G;

location / {
autoindex on;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think we can wrap this in if condition. This is for if the index file missing then show the folder content

Suggested change
autoindex on;
if (!$use_front_controller) {
autoindex on;
}


location / {
autoindex on;
try_files $uri $uri/ /index.php?$query_string;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
try_files $uri $uri/ /index.php?$query_string;
# Redirect non-existing files to index.php for "pretty" URLs
try_files $uri $uri/ /index.php?$query_string;

Comment on lines -26 to +38
fastcgi_param SCRIPT_FILENAME $document_root/$fastcgi_script_name;
fastcgi_param SCRIPT_NAME $fastcgi_script_name;

# For v21+ installs (public/ webroot), all PHP requests are routed through
# public/index.php (the front controller). The front controller reads
# REQUEST_URI to dispatch to the correct script inside server/.
# For v13-20 installs the real script exists under server/ and is served directly.
set $script_filename $document_root$fastcgi_script_name;
if ($use_front_controller) {
set $script_filename $document_root/index.php;
}
fastcgi_param SCRIPT_FILENAME $script_filename;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These changes are not necessary as long as you made the change in server.conf

Comment thread nginx/config/server.conf
Comment on lines 27 to +53
# Default root
set $rootdir "$REMOTE_SRC";

if ($sitename != "") {
set $rootdir "$rootdir/$sitename";
}

# If we have the server directory (version 13 and newer) use this as the wwwroot
# If we have the server directory (version 13 to 20) use this as the wwwroot
if (-f "$rootdir/server/version.php") {
# in which case, set that directory as the root
set $rootdir "$rootdir/server";
}

# If we have the public directory (version 21 and newer) it is a sibling of server/,
# so check from the original sitename root. Re-check against REMOTE_SRC base.
# We use a separate variable to avoid double-appending on the server/ branch.
set $publicrootdir "$REMOTE_SRC";
if ($sitename != "") {
set $publicrootdir "$publicrootdir/$sitename";
}

# $use_front_controller is set to 1 for v21+ installs that use a public/ webroot
# and rely on public/index.php as the single entry point for all PHP requests.
set $use_front_controller 0;
if (-d "$publicrootdir/public") {
set $rootdir "$publicrootdir/public";
set $use_front_controller 1;
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggested change
}
# Default root
set $base_rootdir "$REMOTE_SRC";
if ($sitename != "") {
set $base_rootdir "$base_rootdir/$sitename";
}
# If we have the server directory (version 13 to 20) use this as the wwwroot
if (-f "$base_rootdir/server/version.php") {
set $rootdir "$base_rootdir/server";
set $use_front_controller 0;
}
# If we have the public directory (version 21 and newer) it is a sibling of server/,
# so check from the original sitename root. Re-check against REMOTE_SRC base.
# We use a separate variable to avoid double-appending on the server/ branch.
if (-d "$base_rootdir/public") {
set $rootdir "$base_rootdir/public";
set $use_front_controller 1;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I think this will be easier to read

Comment on lines -53 to +67
fastcgi_param SCRIPT_FILENAME $document_root/$fastcgi_script_name;
fastcgi_param SCRIPT_NAME $fastcgi_script_name;

# For v21+ installs (public/ webroot), route all PHP through the front controller.
set $script_filename $document_root$fastcgi_script_name;
if ($use_front_controller) {
set $script_filename $document_root/index.php;
}
fastcgi_param SCRIPT_FILENAME $script_filename;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

These change will not be necessary if you applied the change in server.conf

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.

2 participants