Skip to content

Refactor check_upgrade_savepoint after MDL-83424 #363

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Refactor check_upgrade_savepoint after MDL-83424 #363

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a860058
Remove check for old branch from check_upgrade_savepoints
ziegenberg Jul 18, 2025
20d5053
Added check to verify there is not more than one upgrade function to …
ziegenberg Jul 18, 2025
cf13867
Run phpcbf on check_upgrade_savepoints with moodle code standards
ziegenberg Jul 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 91 additions & 120 deletions check_upgrade_savepoints/check_upgrade_savepoints.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
<?php

// This file is part of Moodle - http://moodle.org/
//
// Moodle is free software: you can redistribute it and/or modify
Expand Down Expand Up @@ -35,73 +34,47 @@
define('LINEFEED', "\n");
}

// Nasty but need this here to include version.php, grrr
define('MOODLE_INTERNAL', 1);
define('MATURITY_ALPHA', 50); // internals can be tested using white box techniques
define('MATURITY_BETA', 100); // feature complete, ready for preview and testing
define('MATURITY_RC', 150); // tested, will be released unless there are fatal bugs
define('MATURITY_STABLE', 200); // ready for production deployment
define('ANY_VERSION', 'any'); // special value that can be used in $plugin->dependencies in version.php files
$plugin = new stdClass();
$module = new stdClass();

// Detect if we are in 2.3 and up by looking for $branch
if (file_exists('version.php')) {
require_once('version.php');
}
$moodle23andup = isset($branch) ? true : false;

$dir = dirname(__FILE__);

$files = files_to_check($dir);

foreach ($files as $file) {
echo " - $file: " . LINEFEED;

$contents = file_get_contents($file);

$function_regexp = '\s*function\s+xmldb_[a-zA-Z0-9_]+?_upgrade\s*\(.*?version.*?\)(?::\sbool)?\s*(?=\{)';
$return_regexp = '\s*return true;';
$anyfunction_regexp = '\s*function\s*[a-z0-9_]+?\s*\(.*?\)\s*{'; // MDL-34103
$functionregexp = '\s*function\s+xmldb_[a-zA-Z0-9_]+?_upgrade\s*\(.*?version.*?\)(?::\sbool)?\s*(?=\{)';
$returnregexp = '\s*return true;';
$anyfunctionregexp = '\s*function\s*[a-z0-9_]+?\s*\(.*?\)\s*{'; // MDL-34103

/// Find we have some xmldb_xxxx_function in code
if (! $countxmldb = preg_match_all('@' . $function_regexp . '@is', $contents, $matches)) {
// Find we have some xmldb_xxxx_function in code
if (! $countxmldb = preg_match_all('@' . $functionregexp . '@is', $contents, $matches)) {
echo " + ERROR: upgrade function not found" . LINEFEED;
continue;
}
/// Verify there is only one upgrade function
// Verify there is only one upgrade function
if ($countxmldb !== 1) {
echo " + ERROR: multiple upgrade functions detected" . LINEFEED;
continue;
}

// These checks are only performed for 23_STABLE and up
if ($moodle23andup) {

// Find we have some return true; in code
if (! $countreturn = preg_match_all('@' . $return_regexp . '@is', $contents, $matches)) {
echo " + ERROR: 'return true;' not found" . LINEFEED;
continue;
}
// Verify there is only one return true;
if ($countreturn !== 1) {
echo " + ERROR: multiple 'return true;' detected" . LINEFEED;
continue;
}

/** Commented till MDL-34103 is fixed to avoid getting fails
// Find we have some function in code
if (! $countfunction = preg_match_all('@' . $anyfunction_regexp . '@is', $contents, $matches)) {
echo " + ERROR: functions not found" . LINEFEED;
continue;
}
// Verify there is only one function
if ($countfunction !== 1) {
echo " + ERROR: multiple functions detected (use upgradelib, plz)" . LINEFEED;
continue;
}
*/
// Find we have some return true; in code
if (! $countreturn = preg_match_all('@' . $returnregexp . '@is', $contents, $matches)) {
echo " + ERROR: 'return true;' not found" . LINEFEED;
continue;
}
// Verify there is only one return true;
if ($countreturn !== 1) {
echo " + ERROR: multiple 'return true;' detected" . LINEFEED;
continue;
}

// Verify there is not more than one function
if (preg_match_all('@' . $anyfunctionregexp . '@is', $contents, $matches) > 1) {
echo " + ERROR: multiple functions detected (use upgradelib, plz)" . LINEFEED;
continue;
}

// Extract all string literals in upgrade code, we are not interested on them and can lead to
Expand All @@ -114,7 +87,7 @@
// In any case, all the replacements performed are stored in $discardedliterals just in case
// something needs to be recovered back.
$regexp = '(["\'])(?:\\\\\1|.)*?\1'; // Match all quoted literals in a text, ignoring escaped ones.
$discardedliterals = array();
$discardedliterals = [];
// Look for all quoted strings.
preg_match_all('@' . $regexp . '@', $contents, $matches);
// Iterate them, keeping safe ones and replacing by placeholder conflictive ones.
Expand All @@ -123,7 +96,7 @@
$unsaferegexp = '[\[\(\{\<\>\}\)\]]'; // Consider everything but [({<>})] safe.
if (preg_match('@' . $unsaferegexp . '@', $string)) {
// The string is not safe, replace it by placeholder and annotate the replacement.
$replacement = "'<%&%" . (string)(count($discardedliterals) +1) . "%&%>'";
$replacement = "'<%&%" . (string)(count($discardedliterals) + 1) . "%&%>'";
$discardedliterals[$replacement] = $string;
} else {
// The string is safe, keep it as is, no need to replace it by placeholder.
Expand All @@ -134,97 +107,97 @@
$contents = str_replace($discardedliterals, array_keys($discardedliterals), $contents);
}

/// Arrived here, extract function contents
if (! preg_match_all('@' . $function_regexp . '.*?(\{(?>(?>[^{}]+)|(?1))*\})@is', $contents, $matches)) {
// Arrived here, extract function contents
if (! preg_match_all('@' . $functionregexp . '.*?(\{(?>(?>[^{}]+)|(?1))*\})@is', $contents, $matches)) {
echo " + NOTE: cannot find upgrade function contents" . LINEFEED;
continue;
}

/// Calculate function contents (must be a group of "if" blocks)
// Calculate function contents (must be a group of "if" blocks)
$contents = trim(trim($matches[1][0], '}{'));

$if_regexp = 'if\s+?\(\s*?\$oldversion\s*?<\s*?([0-9.]{8,13}).*?\)\s*?';
$sp_regexp = 'upgrade_(main|mod|block|plugin)_savepoint\s*?\(\s*?true\s*?,\s*?([0-9.]{8,13})\s*?.*?\);';
$ifregexp = 'if\s+?\(\s*?\$oldversion\s*?<\s*?([0-9.]{8,13}).*?\)\s*?';
$spregexp = 'upgrade_(main|mod|block|plugin)_savepoint\s*?\(\s*?true\s*?,\s*?([0-9.]{8,13})\s*?.*?\);';

/// Count ifs and savepoints. Must match
$count_if = preg_match_all('@' . $if_regexp . '@is', $contents, $matches1);
$count_sp = preg_match_all('@' . $sp_regexp . '@is', $contents, $matches2);
if ($count_if > 0 || $count_sp > 0) {
if ($count_if !== $count_sp) {
if ($count_if < $count_sp) {
echo " + WARN: Detected fewer 'if' blocks ($count_if) than 'savepoint' calls ($count_sp). Repeated savepoints?" . LINEFEED;
// Count ifs and savepoints. Must match
$countif = preg_match_all('@' . $ifregexp . '@is', $contents, $matches1);
$countsp = preg_match_all('@' . $spregexp . '@is', $contents, $matches2);
if ($countif > 0 || $countsp > 0) {
if ($countif !== $countsp) {
if ($countif < $countsp) {
echo " + WARN: Detected fewer 'if' blocks ($countif) than 'savepoint' calls ($countsp). Repeated savepoints?" . LINEFEED;
} else {
echo " + ERROR: Detected more 'if' blocks ($count_if) than 'savepoint' calls ($count_sp)" . LINEFEED;
echo " + ERROR: Detected more 'if' blocks ($countif) than 'savepoint' calls ($countsp)" . LINEFEED;
}
} else {
echo " + found $count_if matching 'if' blocks and 'savepoint' calls" . LINEFEED;
echo " + found $countif matching 'if' blocks and 'savepoint' calls" . LINEFEED;
}
}

/// Let's ensure there are no duplicate calls to a save point with the same version.
if ($count_sp > 0) {
// Let's ensure there are no duplicate calls to a save point with the same version.
if ($countsp > 0) {
foreach (array_count_values($matches2[2]) as $version => $count) {
if ($count > 1) {
echo " + ERROR: Detected multiple 'savepoint' calls for version $version".LINEFEED;
echo " + ERROR: Detected multiple 'savepoint' calls for version $version" . LINEFEED;
}
}
}

/// Let's split them
if (!preg_match_all('@(' . $if_regexp . '(\{(?>(?>[^{}]+)|(?3))*\}))@is', $contents, $matches)) {
// Let's split them
if (!preg_match_all('@(' . $ifregexp . '(\{(?>(?>[^{}]+)|(?3))*\}))@is', $contents, $matches)) {
echo " + NOTE: cannot find 'if' blocks within the upgrade function" . LINEFEED;
continue;
}

$versions = $matches[2];
$blocks = $matches[3];

/// Foreach version, check order
$version_p = 0;
$has_version_error = false;
foreach($versions as $version) {
if (!$version_p) {
$version_p = $version;
// Foreach version, check order
$versionp = 0;
$hasversionerror = false;
foreach ($versions as $version) {
if (!$versionp) {
$versionp = $version;
continue;
}
if (((float)$version * 100) < ((float)$version_p * 100)) {
echo " + ERROR: Wrong order in versions: $version_p and $version" . LINEFEED;
$has_version_error = true;
if (((float)$version * 100) < ((float)$versionp * 100)) {
echo " + ERROR: Wrong order in versions: $versionp and $version" . LINEFEED;
$hasversionerror = true;
}
$version_p = $version;
$versionp = $version;
}
if (!$has_version_error) {
if (!$hasversionerror) {
echo " + versions in upgrade blocks properly ordered" . LINEFEED;
}

/// Foreach version, look for corresponding savepoint
$has_version_mismatch = false;
// Foreach version, look for corresponding savepoint
$hasversionmismatch = false;
foreach ($versions as $key => $version) {
$count_spv = preg_match_all('@' .$sp_regexp . '@is', $blocks[$key], $matches);
if ($count_spv == 0) {
$countspv = preg_match_all('@' . $spregexp . '@is', $blocks[$key], $matches);
if ($countspv == 0) {
echo " + ERROR: version $version is missing corresponding savepoint call" . LINEFEED;
$has_version_mismatch = true;
} else if ($count_spv > 1) {
$hasversionmismatch = true;
} else if ($countspv > 1) {
echo " + ERROR: version $version has more than one savepoint call" . LINEFEED;
$has_version_mismatch = true;
$hasversionmismatch = true;
} else {
if ($version !== $matches[2][0]) {
echo " + ERROR: version $version has wrong savepoint call with version {$matches[2][0]}" . LINEFEED;
$has_version_mismatch = true;
$hasversionmismatch = true;
}
}
}
if (!$has_version_mismatch) {
if (!$hasversionmismatch) {
echo " + versions in savepoint calls properly matching upgrade blocks" . LINEFEED;
}

/// Ensure a plugin does not upgrade past its defined version.
$versionfile = dirname(dirname($file)).'/version.php';
// Ensure a plugin does not upgrade past its defined version.
$versionfile = dirname(dirname($file)) . '/version.php';
if (file_exists($versionfile)) {
if (preg_match('/^\s*\$(module|plugin)->version\s*=\s*([\d.]+)/m', file_get_contents($versionfile), $versionmatches) === 1) {
foreach ($versions as $version) {
if (((float) $versionmatches[2] * 100) < ((float) $version * 100)) {
echo " + ERROR: version $version is higher than that defined in $versionfile file".LINEFEED;
echo " + ERROR: version $version is higher than that defined in $versionfile file" . LINEFEED;
}
}
}
Expand All @@ -234,43 +207,41 @@
/**
* Given one full path, return one array with all the files to check
*/
function files_to_check($path) {

$results = array();
$pending = array();
function files_to_check($path) {

$dir = opendir($path);
while (false !== ($file=readdir($dir))) {
$results = [];
$pending = [];

$fullpath = $path . '/' . $file;
$dir = opendir($path);
while (false !== ($file = readdir($dir))) {
$fullpath = $path . '/' . $file;

if (substr($file, 0, 1)=='.' || $file=='CVS' || $file=='.git') { /// Exclude some dirs
continue;
}

if (is_dir($fullpath)) { /// Process dirs later
$pending[] = $fullpath;
continue;
}
if (substr($file, 0, 1) == '.' || $file == 'CVS' || $file == '.git') { // Exclude some dirs
continue;
}

if (is_file($fullpath) && strpos($file, basename(__FILE__))!==false) { /// Exclude me
continue;
}
if (is_dir($fullpath)) { // Process dirs later
$pending[] = $fullpath;
continue;
}

if (is_file($fullpath) && strpos($fullpath, 'db/upgrade.php')===false) { /// Exclude non upgrade.php files
continue;
}
if (is_file($fullpath) && strpos($file, basename(__FILE__)) !== false) { // Exclude me
continue;
}

if (!in_array($fullpath, $results)) { /// Add file if doesn't exists
$results[$fullpath] = $fullpath;
}
if (is_file($fullpath) && strpos($fullpath, 'db/upgrade.php') === false) { // Exclude non upgrade.php files
continue;
}
closedir($dir);

foreach ($pending as $pend) {
$results = array_merge($results, files_to_check($pend));
if (!in_array($fullpath, $results)) { // Add file if doesn't exists
$results[$fullpath] = $fullpath;
}
}
closedir($dir);

return $results;
foreach ($pending as $pend) {
$results = array_merge($results, files_to_check($pend));
}
?>

return $results;
}
Loading