Skip to content

Commit fbea2bf

Browse files
Rom1-BRom1-B
committed
review 🦈
1 parent 1f9fcbe commit fbea2bf

2 files changed

Lines changed: 45 additions & 13 deletions

File tree

‎inc/container.class.php‎

Lines changed: 21 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -973,7 +973,7 @@ public static function preItemPurge($item)
973973
{
974974
$itemtype = $item::class;
975975
$containers = new self();
976-
$founded_containers = $containers->find();
976+
$founded_containers = $containers->find(['is_active' => 1]);
977977
foreach ($founded_containers as $container) {
978978
$itemtypes = PluginFieldsToolbox::decodeJSONItemtypes($container['itemtypes']);
979979
if (in_array($itemtype, $itemtypes)) {
@@ -1030,7 +1030,8 @@ public static function renameOversizedContainer(int $id, string $new_name): bool
10301030
return false;
10311031
}
10321032

1033-
$new_name = preg_replace('/[^\da-zA-Z]/', '', $new_name) ?? '';
1033+
$toolbox = new PluginFieldsToolbox();
1034+
$new_name = $toolbox->getSystemNameFromLabel($new_name);
10341035
if ($new_name === '') {
10351036
Session::AddMessageAfterRedirect(__('Invalid name.', 'fields'), false, ERROR);
10361037

@@ -1081,7 +1082,7 @@ public static function renameOversizedContainer(int $id, string $new_name): bool
10811082

10821083
$old_table = getTableForItemType(self::getClassname($itemtype, $old_name));
10831084
if (!$DB->tableExists($old_table)) {
1084-
$old_table = self::findOrphanTableForContainer($id, $claimed_orphans);
1085+
$old_table = self::findOrphanTableForContainer($id, $itemtype, $claimed_orphans);
10851086
}
10861087

10871088
if ($old_table === null || !$DB->tableExists($old_table)) {
@@ -1124,9 +1125,10 @@ public static function renameOversizedContainer(int $id, string $new_name): bool
11241125
* Find an orphaned table belonging to this container.
11251126
*
11261127
* @param int $container_id Container ID.
1128+
* @param string $itemtype Item type the orphan table must have been generated for.
11271129
* @param string[] $already_claimed Orphan tables already assigned to another itemtype in this call.
11281130
*/
1129-
private static function findOrphanTableForContainer(int $container_id, array $already_claimed): ?string
1131+
private static function findOrphanTableForContainer(int $container_id, string $itemtype, array $already_claimed): ?string
11301132
{
11311133
/** @var DBmysql $DB */
11321134
global $DB;
@@ -1136,16 +1138,27 @@ private static function findOrphanTableForContainer(int $container_id, array $al
11361138
$already_claimed,
11371139
);
11381140

1139-
// Primary match: `plugin_fields_containers_id` is DEFAULTed to the container's own id
1140-
// at table creation time, a link that survives any later name corruption.
1141+
// Primary match: `plugin_fields_containers_id` and `itemtype` are both DEFAULTed
1142+
// at table creation time, a link that survives any later name corruption. Matching
1143+
// on both is required when a container spans several item types, since every one
1144+
// of its orphan tables shares the same `plugin_fields_containers_id` default.
11411145
$by_default = [];
11421146
foreach ($orphaned as $table) {
1147+
$matches_container = false;
1148+
$matches_itemtype = false;
11431149
foreach ($DB->listFields($table) as $column) {
11441150
if ($column['Field'] === 'plugin_fields_containers_id' && (int) $column['Default'] === $container_id) {
1145-
$by_default[] = $table;
1146-
break;
1151+
$matches_container = true;
1152+
}
1153+
1154+
if ($column['Field'] === 'itemtype' && $column['Default'] === $itemtype) {
1155+
$matches_itemtype = true;
11471156
}
11481157
}
1158+
1159+
if ($matches_container && $matches_itemtype) {
1160+
$by_default[] = $table;
1161+
}
11491162
}
11501163

11511164
if (count($by_default) === 1) {

‎tests/Units/ContainerTest.php‎

Lines changed: 24 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
use PluginFieldsContainer;
4545
use PluginFieldsDropdown;
4646
use PluginFieldsField;
47+
use PluginFieldsToolbox;
4748
use Session;
4849
use Ticket;
4950
use UserEmail;
@@ -330,17 +331,18 @@ public function testRenameOversizedContainerSucceeds(): void
330331
global $DB;
331332
$DB->update(PluginFieldsContainer::getTable(), ['name' => str_repeat('a', 100)], ['id' => $container->getID()]);
332333

333-
$new_name = 'Renamed' . str_replace('-', '', $this->getUniqueString());
334+
$new_name = 'Renamed' . str_replace('-', '', $this->getUniqueString());
335+
$expected_new_name = (new PluginFieldsToolbox())->getSystemNameFromLabel($new_name);
334336

335337
$result = PluginFieldsContainer::renameOversizedContainer($container->getID(), $new_name);
336338
$this->assertTrue($result);
337339

338340
$reloaded = new PluginFieldsContainer();
339341
$this->assertTrue($reloaded->getFromDB($container->getID()));
340-
$this->assertSame($new_name, $reloaded->fields['name']);
342+
$this->assertSame($expected_new_name, $reloaded->fields['name']);
341343
$this->assertSame(1, (int) $reloaded->fields['is_active']);
342344

343-
$table = getTableForItemType(PluginFieldsContainer::getClassname(Computer::class, $new_name));
345+
$table = getTableForItemType(PluginFieldsContainer::getClassname(Computer::class, $expected_new_name));
344346
$this->assertTrue($DB->tableExists($table));
345347

346348
// The reactivated container must be genuinely usable, not just flagged active.
@@ -440,16 +442,24 @@ public function testRenameOversizedContainerFailsWhenNameIsStillTooLong(): void
440442
'entities_id' => 0,
441443
'is_recursive' => 1,
442444
]);
443-
$original_name = $container->fields['name'];
445+
$short_name = $container->fields['name'];
446+
447+
/** @var DBmysql $DB */
448+
global $DB;
449+
$DB->update(PluginFieldsContainer::getTable(), ['name' => str_repeat('a', 100)], ['id' => $container->getID()]);
444450

445451
$result = PluginFieldsContainer::renameOversizedContainer($container->getID(), str_repeat('a', 100));
446452

447453
$this->assertFalse($result);
448454

449455
$reloaded = new PluginFieldsContainer();
450456
$this->assertTrue($reloaded->getFromDB($container->getID()));
451-
$this->assertSame($original_name, $reloaded->fields['name']);
457+
$this->assertSame(str_repeat('a', 100), $reloaded->fields['name']);
452458
$this->assertSame(0, (int) $reloaded->fields['is_active']);
459+
460+
// Restore a table-name-safe value so the deletion done in tearDown does not
461+
// itself attempt a DROP TABLE with an over-64-char identifier.
462+
$DB->update(PluginFieldsContainer::getTable(), ['name' => $short_name], ['id' => $container->getID()]);
453463
}
454464

455465
public function testRenameOversizedContainerFailsOnNameCollision(): void
@@ -471,6 +481,11 @@ public function testRenameOversizedContainerFailsOnNameCollision(): void
471481
'entities_id' => 0,
472482
'is_recursive' => 1,
473483
]);
484+
$short_name = $container->fields['name'];
485+
486+
/** @var DBmysql $DB */
487+
global $DB;
488+
$DB->update(PluginFieldsContainer::getTable(), ['name' => str_repeat('a', 100)], ['id' => $container->getID()]);
474489

475490
$result = PluginFieldsContainer::renameOversizedContainer($container->getID(), $existing->fields['name']);
476491

@@ -479,5 +494,9 @@ public function testRenameOversizedContainerFailsOnNameCollision(): void
479494
$reloaded = new PluginFieldsContainer();
480495
$this->assertTrue($reloaded->getFromDB($container->getID()));
481496
$this->assertSame(0, (int) $reloaded->fields['is_active']);
497+
498+
// Restore a table-name-safe value so the deletion done in tearDown does not
499+
// itself attempt a DROP TABLE with an over-64-char identifier.
500+
$DB->update(PluginFieldsContainer::getTable(), ['name' => $short_name], ['id' => $container->getID()]);
482501
}
483502
}

0 commit comments

Comments
 (0)