Skip to content

Fix validation findings - #12

Merged
bonelifer merged 1 commit into
mainfrom
fix/validation-findings
Oct 7, 2026
Merged

bonelifer merged 1 commit into
mainfrom
fix/validation-findings

Conversation

@bonelifer

Copy link
Copy Markdown
Contributor

Fixes the findings from a validation pass run against the phpBB.com validation policy and coding guidelines.

The deny finding

  • ACP permission labels were hardcoded English. They now come from ACL_U_DOCUMENTATION_LANG and ACL_U_DOCUMENTATION_SECTION in the language file.

Behavior changes

  • Default build path moves to store/phpbbmodders_documentation. The old default sat inside the extension's own folder, which is deleted when the extension is updated. phpBB's own deny rules already cover store/. New migration m7_store_docs_path switches the setting only if it is still the old default and no build exists there, so working boards keep their path.
  • Build directory names are validated. Language and section directories must use [a-z0-9_-], fit phpBB's 50-character auth_option column, and a section can't start with lang_ (it would share a permission name with a language). Skipped names are listed on the ACP settings page.
  • Guests without access get phpBB's login_box() instead of a hand-built redirect.
  • The language cookie is set with $user->set_cookie(), so it follows the board's cookie settings. The cookie name changes to <cookie_name>_docs_lang.
  • phpBB is capped at 3.3.x, which is all CI tests: extra.soft-require is now >=3.3.19,<4.0.0@dev, phpbb/phpbb is removed from require, and is_enableable() checks the same range.

Cleanups

  • The controller uses the language service, drops two unused constructor arguments, and checks file size instead of using @getimagesize(). The image page parameter is now read as multibyte.
  • Section and docs-root lookups are cached per request; the navbar used to rescan the build on every page view.
  • Parameters are explicitly nullable (?array), which PHP 8.4 requires to avoid deprecation notices.
  • Values derived from the build are escaped in templates.
  • The notification type uses public static and &amp; in its URL.
  • The JavaScript uses tabs and braces, and a malformed tooltip attribute no longer breaks the script.
  • license.txt now matches the skeleton's.
  • The README, docs/securing-docs-build.md and the contrib/ snippets are updated for the new path.

Testing
Claude added regression tests for each fix (104 → 116 tests) and ran them in Docker on PHP 8.2 and 8.4, with none skipped. Claude also ran the phpbb-release-tools smoke and feature checks on a 3.3.19 board. The feature checks need phpbbmodders/phpbb-release-tools branch documentation-configured-build-path, because the old check hardcoded the previous build path. EPV gives the same result as on main: 0 errors and 6 warnings, none new. Claude did not run this on phpBB 4.0.


Investigated and written by Claude on behalf of William Jacoby (bonelifer).

- Move the ACP permission labels into the language file (the one
  valdeny): listener formats ACL_U_DOCUMENTATION_LANG/_SECTION.
- Skip build directories whose names can't become ACL options (charset,
  50-char auth_option limit, lang_ sections), and list them in the ACP.
- Default the docs build path to store/phpbbmodders_documentation via a
  new migration; boards with a build at the old default keep it.
- Set the language cookie with $user->set_cookie(), use login_box() for
  guests, inject the language service, guard getimagesize() by size
  instead of @, read the image page parameter as multibyte.
- Cache section and docs-root lookups per request; explicit nullable
  parameters for PHP 8.4.
- Escape build-derived values in templates; public static and &amp; in
  the notification type; tabs and braces in the JavaScript.
- Cap phpBB at 3.3.x (soft-require and is_enableable), match the
  skeleton license, consistent array syntax.
- Regression tests for each fix.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@bonelifer
bonelifer requested a review from kaileymsnay October 6, 2026 19:03
@bonelifer
bonelifer merged commit b43a217 into main Oct 7, 2026
7 checks passed
@bonelifer
bonelifer deleted the fix/validation-findings branch October 7, 2026 13:39
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.

1 participant