From 7b0361f37bb8b1b1b7f82be99713d71eed5263f8 Mon Sep 17 00:00:00 2001 From: Marc Durdin Date: Fri, 11 Sep 2026 07:45:27 +0200 Subject: [PATCH] maint: rework PHP error reporting for link tester Setup a more reliable PHP error reporting mechanism when running link test. This helps to capture PHP error messages as the link checker hits each page, providing some basic runtime validation of the site. KeymanSentry.php: With this new test setup, we no longer force pages to exit with 500 errors if any PHP warning or message is encountered (serious errors will still cause a 500 error as is the default). This makes the link checker faster (no retries needed), as all the messages are reported in the do_test_print_container_error_logs function afterwards anyway. Also add composer setup and teardown functions for manual maintenance of composer packages; these can be run through build.sh on the various sites. Test-bot: skip --- .bootstrap-registry | 1 + .gitattributes | 10 +++++++ _common/KeymanSentry.php | 15 +--------- _common/docker.inc.sh | 57 ++++++++++++++++++++++++++++++++++++++ _common/tests.container.sh | 41 +++++++++++++++++++++++++++ _common/tests.inc.sh | 28 ++++++++++++------- bootstrap.inc.sh | 2 +- 7 files changed, 129 insertions(+), 25 deletions(-) create mode 100644 .gitattributes create mode 100755 _common/tests.container.sh diff --git a/.bootstrap-registry b/.bootstrap-registry index 7f40dcb..624d06f 100644 --- a/.bootstrap-registry +++ b/.bootstrap-registry @@ -21,3 +21,4 @@ KeymanSentry.php KeymanVersion.php MarkdownHost.php tests.inc.sh +tests.container.sh diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 0000000..22ad2cc --- /dev/null +++ b/.gitattributes @@ -0,0 +1,10 @@ +# Always use LF for Dockerfile - otherwise HEREDOCs in Dockerfile +# will not work on Windows - we end up with a filename `foo\r` for +# the command +# ```Dockerfile +# RUN < foo +# echo "Hello world" +# EOF +# ``` +Dockerfile text eol=lf +*.sh text eol=lf diff --git a/_common/KeymanSentry.php b/_common/KeymanSentry.php index 14e2416..bca3c38 100644 --- a/_common/KeymanSentry.php +++ b/_common/KeymanSentry.php @@ -7,29 +7,16 @@ require_once __DIR__ . '/KeymanHosts.php'; require_once __DIR__ . '/Assets.php'; - if(KeymanHosts::Instance()->Tier() == KeymanHosts::TIER_DEVELOPMENT || - KeymanHosts::Instance()->Tier() == KeymanHosts::TIER_TEST) { - // For testing broken pages, we want to send HTTP 500 so that - // broken-link-checker will report it; in order to do this we - // need to cache the page output so that headers are not sent - // too early - ob_start(); - } - class KeymanSentry { static function init($dsn) { \Sentry\init([ 'dsn' => $dsn, 'environment' => KeymanHosts::Instance()->TierName(), + // 'logger' => new \Sentry\Logger\DebugStdOutLogger(), 'before_send' => function (\Sentry\Event $event) { // Don't send events from localhost or dev environments if (KeymanHosts::Instance()->Tier() == KeymanHosts::TIER_DEVELOPMENT || KeymanHosts::Instance()->Tier() == KeymanHosts::TIER_TEST) { - if(headers_sent()) { - echo "

Fatal error: not setting 500 because headers already sent.

"; - } else { - header("HTTP/1.1 500 Internal Server Error"); - } return null; } return $event; diff --git a/_common/docker.inc.sh b/_common/docker.inc.sh index ff0aa0e..70471de 100644 --- a/_common/docker.inc.sh +++ b/_common/docker.inc.sh @@ -81,6 +81,9 @@ function build_docker_container() { builder_echo "Building using $BUILDER_CONFIGURATION configuration" # Download docker image. --mount option requires BuildKit + # + # note: --no-cache may be added here if trying to replicate broken ci locally + # where Docker resources have been cached DOCKER_BUILDKIT=1 $CONTAINER_ENGINE build -t $IMAGE_NAME --build-arg BUILDER_CONFIGURATION="${BUILDER_CONFIGURATION}" $FILE $TARGET } @@ -188,6 +191,7 @@ function test_docker_container() { local LINK_RESULT=0 echo "TIER_TEST" > tier.txt + trap _docker_cleanup_test_docker_container ERR EXIT SIGINT SIGTERM # Similar pattern in ci.yml on sites @@ -206,17 +210,70 @@ function test_docker_container() { if ! builder_has_option --no-link-check; then builder_echo blue "---- Testing links" + do_test_links_setup "${CONTAINER_DESC}" + do_test_links "http://localhost:${CONTAINER_PORT}" "$TEST_PATH" "${SKIP_PATHS[@]}" || LINK_RESULT=$? builder_echo blue "Done checking links; linkinator exit code: ${LINK_RESULT}" do_test_print_link_report do_test_print_container_error_logs "${CONTAINER_DESC}" + + do_test_links_cleanup "${CONTAINER_DESC}" fi rm tier.txt return "$LINK_RESULT" } +_docker_cleanup_test_docker_container() { + rm -f "$THIS_SCRIPT_PATH/tier.txt" +} + +# +# Setup a temporary container for interactive composer updates +# +function docker_build_and_start_composer_container() { + local COMPOSER_ID=composer-temp + docker build -f Dockerfile --target composer-builder --tag $COMPOSER_ID . + + local DOCKER_BINDING + if [[ $OSTYPE =~ msys|cygwin ]]; then + # Windows needs leading slashes for path + DOCKER_BINDING="//$(pwd):/var/www/html/" + else + DOCKER_BINDING="$(pwd):/var/www/html/" + fi + + docker run -v "$DOCKER_BINDING" --name $COMPOSER_ID --user root --rm -d $COMPOSER_ID + + echo + echo "Some of the following commands may be helpful:" + echo " docker exec $COMPOSER_ID composer audit" + echo " docker exec $COMPOSER_ID composer update" + echo " docker exec $COMPOSER_ID composer update --lock" + echo " docker exec $COMPOSER_ID composer require package-name" + echo " docker exec $COMPOSER_ID composer require package-name --dev" + echo +} + +# +# Cleanup the temporary container used for interactive composer updates +# (including copying composer files) +# +function docker_stop_and_cleanup_composer_container() { + local COMPOSER_ID=composer-temp + # copy modified files to mounted volume: + docker exec $COMPOSER_ID cp composer.lock //var/www/html/ + docker exec $COMPOSER_ID cp composer.json //var/www/html/ + # cleanup + docker stop $COMPOSER_ID + docker rmi $COMPOSER_ID + + builder_echo "Copied any changes from composer.json, composer.lock from init container into repo" + builder_echo "Note: 'build.sh build' will be required if changes have been made to /vendor by composer" +} + + # Returns 0 if the specified container engine is available, 1 otherwise _is_container_engine() { diff --git a/_common/tests.container.sh b/_common/tests.container.sh new file mode 100755 index 0000000..954a355 --- /dev/null +++ b/_common/tests.container.sh @@ -0,0 +1,41 @@ +#!/bin/bash +# +# Keyman is copyright (C) SIL Global. MIT License. +# +# Scripts to run in container for link tests -- capture all PHP messages emitted +# when each page is visited by the link checker, and report on errors +# + +set -eu + +if [[ ${BUILDER_PLATFORM-x} != docker ]]; then + echo tests.container.sh should run only in the Docker container context + exit 1 +fi + +if [ $# -lt 1 ]; then + echo "usage: $0 setup|report|cleanup" + exit 65 +fi + +ERROR_LOG=/tmp/php_errors.log + +if [ "$1" == "setup" ]; then + cp /usr/local/etc/php/php.ini-development /usr/local/etc/php/php.ini + echo "error_log = $ERROR_LOG" >> /usr/local/etc/php/php.ini + # note: apache restart must run from docker host + rm -f $ERROR_LOG +elif [ "$1" == "report" ]; then + if [ -f $ERROR_LOG ]; then + cat $ERROR_LOG + exit 1 + fi +elif [ "$1" == "cleanup" ]; then + cp /usr/local/etc/php/php.ini-development /usr/local/etc/php/php.ini + # note: apache restart must run from docker host +else + echo "Invalid parameter" + exit 65 +fi + +exit 0 \ No newline at end of file diff --git a/_common/tests.inc.sh b/_common/tests.inc.sh index 8dfe344..e34786a 100644 --- a/_common/tests.inc.sh +++ b/_common/tests.inc.sh @@ -33,7 +33,7 @@ function do_test_lint() { ## Check links on live local server using linkinator # # Parameters -# 1: baseURL the top level URL for the site +# 1: baseURL the top level URL for the site, e.g. http://localhost:8053 (no terminating slash) # 2: testPath path under baseURL to start testing, e.g. / # 3[,4..]: skipPaths list of paths (under baseURL) to skip crawling, optional # @@ -48,6 +48,8 @@ function do_test_links() { skipParams+=(--skip "^${baseURL}${skip}") done + builder_echo "Testing links; --skip ^(?!${baseURL}) ${skipParams[*]}" + npx https://github.com/keymanapp/linkinator \ "${baseURL}${testPath}" \ --clean-urls \ @@ -76,19 +78,25 @@ function do_test_print_link_report() { } # Scan logs recorded on container since start of tests to find any reported PHP -# errors (note, depends on '[php#:xxxx]' marker string, where # = 7 for PHP7, omitted for PHP8) +# errors # # Parameters # 1: CONTAINER container_desc to run on # function do_test_print_container_error_logs() { local CONTAINER="$1" - if docker container logs "${CONTAINER}" --since "${TEST_START_TIME}" 2>&1 | grep -qP '\[php7?:(error|warn|notice)\]'; then - echo 'PHP reported errors or warnings:' - docker container logs "${CONTAINER}" --since "${TEST_START_TIME}" 2>&1 | grep -P '\[php7?:(error|warn|notice)\]' - return 1 - else - echo 'No PHP errors found' - return 0 - fi + docker exec "${CONTAINER}" //var/www/html/_common/tests.container.sh report } + + +function do_test_links_setup() { + local CONTAINER="$1" + docker exec "${CONTAINER}" //var/www/html/_common/tests.container.sh setup + docker kill "${CONTAINER}" --signal="USR1" +} + +function do_test_links_cleanup() { + local CONTAINER="$1" + docker exec "${CONTAINER}" //var/www/html/_common/tests.container.sh cleanup + docker kill "${CONTAINER}" --signal="USR1" +} \ No newline at end of file diff --git a/bootstrap.inc.sh b/bootstrap.inc.sh index 834c5b0..342ae4a 100644 --- a/bootstrap.inc.sh +++ b/bootstrap.inc.sh @@ -215,7 +215,7 @@ else BUILDER_TIER=TIER_DEVELOPMENT fi -if [[ "$BUILDER_TIER" == TIER_DEVELOPMENT ]]; then +if [[ "$BUILDER_TIER" == TIER_DEVELOPMENT || "$BUILDER_TIER" == TIER_TEST ]]; then export KEYMAN_VERSION_ENVIRONMENT=local fi