fix: handle redirects for paths and files with terminating slashes 🌋 by mcdurdin · Pull Request #717 · keymanapp/keyman.com

mcdurdin · 2026-04-30T13:55:42Z

Remove the / from a path if it refers to a file, and add a / if the path refers to a folder (implicit index.md/index.php).

Clean up keyboard search which had some assumptions about not including the / but that doesn't make sense with this pattern.

Test-bot: skip

keymanapp-test-bot · 2026-04-30T13:55:46Z

User Test Results

Test specification and instructions

User tests are not required

mcdurdin · 2026-05-05T14:41:59Z


  <div class="download-cta-small iPhone" id="cta-iPhone">
-    <a href="../../../iphone/">
+    <a href="../../../iphone-and-ipad/">


We keep /ipad and /iphone as top-level redirects, but we don't need those for e.g. /en/ipad as the content is all at /_content/iphone-and-ipad. This eliminates a rule in the .htaccess

mcdurdin · 2026-05-05T14:43:24Z

      <label for="search-q"><?= _m('keyboard_search') ?></label><input id="search-q" type="text" placeholder="<?= _m('enter_language') ?>" name="q"
      <?php if($embed == 'none') echo 'autofocus'; ?>>
      <input id="search-f" type="button" value="<?= _m('search') ?>">
-      <label id="search-new"><a href='/<?= $head_options['language']?>/keyboards<?=$session_query_q?>'><?= _m('new_search')?></a></label>


This line was broken (nested <?=), when I fixed it I decided to simplify all references

mcdurdin · 2026-05-05T14:46:17Z

-# ------------------------------------------------------------------
-# top-level content without i18n - _control/, go/, _test/, cdn
-# ------------------------------------------------------------------
-
-# No i18n - won't go to _content
-
-RewriteCond "%{DOCUMENT_ROOT}/$1/$2.php" -f
-RewriteRule "^(_control|go|_test)/(.+)$" "/$1/$2.php" [END]
-
-RewriteCond "%{DOCUMENT_ROOT}/$1/$2.md" -f
-RewriteRule "^(_control|go|_test)/(.+)$" "/_includes/includes/md/mdhost.php?file=$1/$2.md" [END]
-
-RewriteCond "%{DOCUMENT_ROOT}/$1/index.md" -f
-RewriteRule "^(_control|go|_test)(/?)$" "/_includes/includes/md/mdhost.php?file=$1/index.md" [END]
-
-RewriteCond "%{DOCUMENT_ROOT}/cdn/$1$2" -f
-RewriteRule "^cdn/(.+\.)(gif|js|png|svg)$" "/cdn/$1$2" [END]


This section is now moved up

mcdurdin · 2026-05-05T14:46:48Z

-RewriteRule "^(([a-z]{2,3})(-([A-Za-z]{4}))?(-([a-z]{2}|[0-9]{3}))?)/keyboards/install/([^/]+)$" "/_content/keyboards/install.php?id=$7&lang=$1" [NC,END,QSA]
+RewriteCond "$1" ^([a-z]{2,3})(-([A-Za-z]{4}))?(-([a-z]{2}|[0-9]{3}))?$ [NC]   # BCP 47 match
+RewriteRule "^([^/]+)/keyboards/install/([^/]+)$" "/_content/keyboards/install.php?id=$2&lang=$1" [END,QSA]


Hmm, was going to put these into a separate PR ... but was working on them and pushed accidentally. Let's see how they go!

darcywong00

ahh, nice cleanup! thanks

darcywong00 · 2026-05-05T20:16:11Z

+#    TODO.md
+#
+RewriteRule "^(_common|_content|_includes|_scripts|.github|resources|tests)(/.*|$)" - [F,END]
+RewriteRule "^(.editorconfig|.gitattributes|.gitignore|build.sh|composer.json|composer.lock|crowdin.yml|Dockerfile|package-lock.json|package.json|phpunit.xml|README.md|TODO.md)$" [F,END]


Does this need - [F,END] like in the other lines?

darcywong00 · 2026-05-05T20:28:42Z

+RewriteCond "$1" ^(_control|_test|_ie_thunk|.well-known|go|cdn)$
+RewriteRule "^([^/]+)$" "$1/" [R,L]
+
+# .php rewrite
+RewriteCond "$1" ^(_control|_test|_ie_thunk)$
+RewriteCond "%{DOCUMENT_ROOT}/$1/$2.php" -f
+RewriteRule "^([^/]+)/(.+)$" "/$1/$2.php" [END]
+
+# .md rewrite
+RewriteCond "$1" ^(_control|_test|_ie_thunk)$
+RewriteCond "%{DOCUMENT_ROOT}/$1/$2.md" -f
+RewriteRule "^([^/]+)/(.+)$" "/_includes/includes/md/mdhost.php?file=$1/$2.md" [END]
+
+# .md rewrite for folder; no .php rewrite for folder
+RewriteCond "$1" ^(_control|_test|_ie_thunk)$
+RewriteCond "%{DOCUMENT_ROOT}/$1/index.md" -f
+RewriteRule "^([^/]+)/$" "/_includes/includes/md/mdhost.php?file=$1/index.md" [END]
+
+# Any existing file in any of those folders or sub folders
+RewriteCond "$1" ^(_control|_ie_thunk|_test|.well-known|go|cdn)$


For sure, was aiming for alphabetical but moved some things around and missed those. Will cleanup

darcywong00 · 2026-05-05T20:33:38Z


-# Redirect folder without / to include /
+# ------------------------------------------------------------------
+# Add and remove `/` for folders and files for consistency


maybe wordsmithing something like

Suggested change

# Add and remove `/` for folders and files for consistency

# For trailing '/' consistency: Add '/' for folders and remove `/` for files

darcywong00 · 2026-05-06T07:05:23Z

    Menu::render([]); // we'll be doing client-side os detection now
  Body::render();
+
+  $keyboardsPage = '/' . $head_options['language'] . '/keyboards';


Devin asks if $keyboardPage missing a trailing slash causes unnecessary 302 redirects.

New .htaccess rules at lines 62-64 redirect /en/keyboards -> /en/keyboards/ because _content/keyboards is a directory containing index.php
...
When a user presses Enter in the search box, this causes an unnecessary redirect from /en/keyboards?q=... to /en/keyboards/?q=...

Remove the `/` from a path if it refers to a file, and add a `/` if the path refers to a folder (implicit index.md/index.php). Clean up keyboard search which had some assumptions about not including the `/` but that doesn't make sense with this pattern. Test-bot: skip

darcywong00

lgtm

keymanapp-test-bot Bot added the epic-i18n-url-scheme label Apr 30, 2026

keymanapp-test-bot Bot changed the title ~~fix: handle redirects for paths and files with terminating slashes~~ fix: handle redirects for paths and files with terminating slashes 🌋 Apr 30, 2026

keymanapp-test-bot Bot added this to the A19S28 milestone Apr 30, 2026

github-actions Bot added the fix label Apr 30, 2026

keyman-status added this to Keyman Apr 30, 2026

github-project-automation Bot moved this to Todo in Keyman Apr 30, 2026

mcdurdin force-pushed the fix/redirects-for-paths branch 3 times, most recently from 95b97ef to 3471e04 Compare April 30, 2026 15:30

darcywong00 reviewed May 3, 2026

View reviewed changes

Comment thread .htaccess

mcdurdin force-pushed the fix/redirects-for-paths branch 3 times, most recently from a60dbfd to c265b3c Compare May 5, 2026 14:07

mcdurdin marked this pull request as ready for review May 5, 2026 14:41

mcdurdin force-pushed the fix/redirects-for-paths branch from c265b3c to 6c2562b Compare May 5, 2026 14:45

mcdurdin commented May 5, 2026

View reviewed changes

darcywong00 approved these changes May 5, 2026

View reviewed changes

darcywong00 reviewed May 6, 2026

View reviewed changes

mcdurdin commented May 6, 2026

View reviewed changes

Comment thread cdn/dev/keyboard-search/search.mjs

mcdurdin force-pushed the fix/redirects-for-paths branch from 6c2562b to dea9d6b Compare May 6, 2026 07:28

darcywong00 approved these changes May 6, 2026

View reviewed changes

mcdurdin merged commit de20356 into fix/search-json May 6, 2026
5 checks passed

mcdurdin deleted the fix/redirects-for-paths branch May 6, 2026 07:51

github-project-automation Bot moved this from Todo to Done in Keyman May 6, 2026

	# Add and remove `/` for folders and files for consistency
	# For trailing '/' consistency: Add '/' for folders and remove `/` for files

Uh oh!

Conversation

mcdurdin commented Apr 30, 2026

Uh oh!

keymanapp-test-bot Bot commented Apr 30, 2026

User Test Results

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

darcywong00 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

darcywong00 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants