Skip to content

CFM-555_Fix_TypeError_and_maximum_bound_handling_in_assignCollisionNumber #459

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
+36 −23
Open

CFM-555_Fix_TypeError_and_maximum_bound_handling_in_assignCollisionNumber #459

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
59 changes: 36 additions & 23 deletions app/plugins/CoreAssigner/src/Model/Table/FormatAssignersTable.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,8 @@
use Cake\ORM\Table;
use Cake\Validation\Validator;
use CoreAssigner\Lib\Enum\CollisionModeEnum;
use InvalidArgumentException;
use Random\RandomException;

class FormatAssignersTable extends Table {
use \App\Lib\Traits\AutoViewVarsTrait;
Expand Down Expand Up @@ -194,11 +196,11 @@ public function assign($ia, $entity): string {
* @since COmanage Registry v5.0.0
* @param int $formatAssignerId Format Assigner ID
* @param string $sequenced Sequenced string as returned by selectSequences()
* @param CollisionModeEnum $collisionMode Collision number assignment mode
* @param string $collisionMode Collision number assignment mode
* @param int $min Minimum number to assign
* @param int $max Maximum number to assign (for Random mode only)
* @param int|null $max Maximum number to assign (for Random mode only)
* @return string Candidate string, possibly with a collision number assigned
* @throws InvalidArgumentException
* @throws InvalidArgumentException|RandomException
*/

protected function assignCollisionNumber(
Expand All @@ -218,25 +220,36 @@ protected function assignCollisionNumber(
switch($collisionMode) {
case CollisionModeEnum::Random:
// Simply pick a number between $min and $max.
$lmax = mt_getrandmax();

$lmax = $max;

if(!$max) {
// We have to be a bit careful with min and max vs mt_rand(). substituteParameters()
if(!empty($max) && $max < $lmax) {
// Smaller configured Maximum
$lmax = $max;
}

if(str_contains($sequenced, '%s')) {
// Unbounded collision placeholder
return sprintf($sequenced, random_int($min, $lmax));
} elseif(preg_match('/\%[0-9.]+s/', $sequenced, $matches)) {
// We have to be a bit careful with min and max. substituteParameters()
// will generate something like (%05.5s). If no explicit $max is configured by the
// admin, we used mt_getrandmax. However, that could generate a string like 172500398.
// We take the first (eg) 5 digits, which are "17250". If $min is 20000, we'll
// incorrectly assign a collision number outside the permitted range (CO-1933).
// admin, we default to mt_getrandmax. However, that could generate a string like 172500398.
// When formatted with width truncation, taking the first (eg) 5 digits ("17250")
// could result in a collision number outside the permitted range if $min is 20000 (CO-1933).

// Pull the width out of the string
$width = (int)rtrim(ltrim(strstr($matches[0], '.'), "."), "s");

// And calculate a new max
$lmax = (10 ** $width) - 1;
}
$width = (int)rtrim(ltrim(strstr($matches[0], '.'), '.'), 's');
$wmax = (10 ** $width) - 1;

$n = random_int($min, $lmax);
return sprintf($sequenced, $n);
if($wmax < $lmax) {
// Smaller implicit maximum from width
$lmax = $wmax;
}

return sprintf($sequenced, random_int($min, $lmax));
} else {
return $sequenced;
}
break;
case CollisionModeEnum::Sequential:
return sprintf($sequenced, $this->FormatAssignerSequences->next(
Expand All @@ -245,7 +258,7 @@ protected function assignCollisionNumber(
start: $min));
break;
default:
throw new InvalidArgumentException(__d('error', 'unknown', $algorithm));
throw new InvalidArgumentException(__d('error', 'unknown', $collisionMode));
break;
}
} else {
Expand All @@ -261,7 +274,7 @@ protected function assignCollisionNumber(
* @since COmanage Registry v5.0.0
* @param string $base Base string as returned by substituteParameters
* @param int $iteration Iteration number (between 0 and 9)
* @param PermittedCharactersEnum $permitted Acceptable characters for substituted parameters
* @param string $permitted Acceptable characters for substituted parameters
* @return string Format with sequenced segments selected
*/

Expand All @@ -273,7 +286,7 @@ protected function selectSequences(
$sequenced = "";

// Loop through the string
for($j = 0;$j < strlen($base);$j++) {
for($j = 0, $jMax = strlen($base); $j < $jMax; $j++) {
Comment on lines -276 to +289
Copy link
Contributor

@benno benno Oct 1, 2026

Choose a reason for hiding this comment

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

How strongly do you feel about rewriting all these for loops?

Commentators in PHP documentation like to overoptimize performance, and calling strlen once per loop is a trivial rounding error in the overall cost of the page load. Personally I find the second line harder to read, but that's just opinion.

switch($base[$j]) {
case '\\':
// Copy the next character directly
Expand Down Expand Up @@ -371,7 +384,7 @@ protected function substituteParameters(
);

// Loop through the format string
for($i = 0;$i < strlen($format);$i++) {
for($i = 0, $iMax = strlen($format); $i < $iMax; $i++) {
switch($format[$i]) {
case '\\':
// Copy the next character directly
Expand All @@ -391,7 +404,7 @@ protected function substituteParameters(
// Check if the next character is a width specifier
if($format[$i+1] == ':') {
// Don't advance $i yet since we still need it, so use $j instead
for($j = $i+2;$j < strlen($format);$j++) {
for($j = $i+2, $jMax = strlen($format); $j < $jMax; $j++) {
if($format[$j] != ')') {
$width .= $format[$j];
} else {
Expand Down