Skip to content
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

Jump to bottom

Add foreign key to unit fields to cleanup if unit is deleted #16836

Open
wants to merge 4 commits into
base: 11.x
Choose a base branch
from
Open

Add foreign key to unit fields to cleanup if unit is deleted #16836

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
66787d1
Add foreign key to unit fields to cleanup if unit is deleted
matthiashamacher Mar 23, 2024
8b93c50
Fix phpstan & sonarcloud issues
matthiashamacher Mar 23, 2024
0f8cbf8
Fix sonarcloud issues
matthiashamacher Mar 23, 2024
b543441
Merge branch '11.x' into feature/quantityvalue-unit-foreign-key
matthiashamacher Apr 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 49 additions & 0 deletions bundles/CoreBundle/src/Migrations/Version20240323121349.php
View file
Open in desktop
@@ -0,0 +1,49 @@
<?php

declare(strict_types=1);

namespace Pimcore\Bundle\CoreBundle\Migrations;

use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;
use Pimcore\Model\DataObject;

final class Version20240323121349 extends AbstractMigration
{
public function getDescription(): string
{
return 'Rebuild classes, objects-bricks and field collections to add foreign key for quantity value units';
}

public function up(Schema $schema): void
{
$classDefinitions = new DataObject\ClassDefinition\Listing();
foreach ($classDefinitions->load() as $classDefinition) {
$this->write(sprintf('Saving class: %s', $classDefinition->getName()));
$classDefinition->save();
}

$objectBricks = new DataObject\Objectbrick\Definition\Listing();
foreach ($objectBricks->load() as $brickDefinition) {
$this->write(sprintf('Saving object brick: %s', $brickDefinition->getKey()));
$brickDefinition->save();
}

$fieldCollections = new DataObject\Fieldcollection\Definition\Listing();
foreach ($fieldCollections->load() as $fieldCollection) {
$this->write(sprintf('Saving field collection: %s', $fieldCollection->getKey()));
$fieldCollection->save();
}
}

public function down(Schema $schema): void
{
$this->write(
sprintf(
'Please restore your class definition files in %s and run
bin/console pimcore:deployment:classes-rebuild manually.',
PIMCORE_CLASS_DEFINITION_DIRECTORY
)
);
}
}
45 changes: 43 additions & 2 deletions models/DataObject/ClassDefinition/Dao.php
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@

namespace Pimcore\Model\DataObject\ClassDefinition;

use Doctrine\DBAL\Exception\DriverException;
use Pimcore\Logger;
use Pimcore\Model;
use Pimcore\Model\DataObject;
Expand Down Expand Up @@ -162,13 +163,33 @@ public function update(): void
// add non existing columns in the table
foreach ($this->model->getFieldDefinitions() as $key => $value) {
if ($value instanceof DataObject\ClassDefinition\Data\ResourcePersistenceAwareInterface
&& $value instanceof DataObject\ClassDefinition\Data) {
&& $value instanceof DataObject\ClassDefinition\Data
) {
// if a datafield requires more than one column in the datastore table => only for non-relation types
if (!$value->isRelationType()) {
if (is_array($value->getColumnType())) {
foreach ($value->getColumnType() as $fkey => $fvalue) {
$this->addModifyColumn($objectDatastoreTable, $key . '__' . $fkey, $fvalue, '', 'NULL');
$protectedDatastoreColumns[] = $key . '__' . $fkey;

if (($value instanceof DataObject\ClassDefinition\Data\QuantityValue
|| $value instanceof DataObject\ClassDefinition\Data\QuantityValueRange)
&& $fkey === 'unit'
) {
try {
$this->db->executeQuery(
sprintf(
'ALTER TABLE `%s` ADD CONSTRAINT `%s` FOREIGN KEY (`%s`)
REFERENCES `quantityvalue_units` (`id`) ON DELETE SET NULL',
$objectDatastoreTable,
self::getForeignKeyName($objectDatastoreTable, $key . '__' . $fkey),
$key . '__' . $fkey
)
);
} catch (DriverException $e) {
// Ignore if the foreign key already exists
}
}
Comment on lines +174 to +192
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance we could somehow extract that into a method? It seems to be same code over and over again.

WE introduced a indexExists method here, instead of just executing the statement and ignoring the error. Maybe we could do the same for foreign keys?

}
} elseif ($value->getColumnType()) {
$this->addModifyColumn($objectDatastoreTable, $key, $value->getColumnType(), '', 'NULL');
Expand All @@ -180,12 +201,32 @@ public function update(): void
}

if ($value instanceof DataObject\ClassDefinition\Data\QueryResourcePersistenceAwareInterface
&& $value instanceof DataObject\ClassDefinition\Data) {
&& $value instanceof DataObject\ClassDefinition\Data
) {
// if a datafield requires more than one column in the query table
if (is_array($value->getQueryColumnType())) {
foreach ($value->getQueryColumnType() as $fkey => $fvalue) {
$this->addModifyColumn($objectTable, $key . '__' . $fkey, $fvalue, '', 'NULL');
$protectedColumns[] = $key . '__' . $fkey;

if (($value instanceof DataObject\ClassDefinition\Data\QuantityValue
|| $value instanceof DataObject\ClassDefinition\Data\QuantityValueRange)
&& $fkey === 'unit'
) {
try {
$this->db->executeQuery(
sprintf(
'ALTER TABLE `%s` ADD CONSTRAINT `%s` FOREIGN KEY (`%s`)
REFERENCES `quantityvalue_units` (`id`) ON DELETE SET NULL',
$objectTable,
self::getForeignKeyName($objectTable, $key . '__' . $fkey),
$key . '__' . $fkey
)
);
} catch (DriverException $e) {
// Ignore if the foreign key already exists
}
}
}
} elseif ($value->getQueryColumnType()) {
$this->addModifyColumn($objectTable, $key, $value->getQueryColumnType(), '', 'NULL');
Expand Down
11 changes: 11 additions & 0 deletions models/DataObject/ClassDefinition/Helper/Dao.php
View file
Open in desktop
Expand Up @@ -119,8 +119,19 @@ protected function removeUnusedColumns(string $table, array $columnsToRemove, ar
//if (!in_array($value, $protectedColumns)) {
if (!in_array(strtolower($value), array_map('strtolower', $protectedColumns))) {
$dropColumns[] = 'DROP COLUMN `' . $value . '`';

if (str_contains($value, 'unit') === true) {
$this->db->executeQuery(
sprintf(
'ALTER TABLE `%s` DROP FOREIGN KEY %s',
$table,
self::getForeignKeyName($table, $value)
)
);
}
Comment on lines +123 to +131
Copy link
Contributor

Choose a reason for hiding this comment

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

If we introduce a foreignKeyExists method, we could also use it here, right?

And I am still not sure about the str_contains check. Could there be some side effects, if there are columns that just happen to have unit in their name?

}
}

if ($dropColumns) {
$this->db->executeQuery('ALTER TABLE `' . $table . '` ' . implode(', ', $dropColumns) . ';');
$this->resetValidTableColumnsCache($table);
Expand Down
20 changes: 20 additions & 0 deletions models/DataObject/Fieldcollection/Definition/Dao.php
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@

namespace Pimcore\Model\DataObject\Fieldcollection\Definition;

use Doctrine\DBAL\Exception\DriverException;
use Pimcore\Model;
use Pimcore\Model\DataObject;

Expand Down Expand Up @@ -75,6 +76,25 @@ public function createUpdateTable(DataObject\ClassDefinition $class): void
foreach ($value->getColumnType() as $fkey => $fvalue) {
$this->addModifyColumn($table, $key . '__' . $fkey, $fvalue, '', 'NULL');
$protectedColums[] = $key . '__' . $fkey;

if (($value instanceof DataObject\ClassDefinition\Data\QuantityValue
|| $value instanceof DataObject\ClassDefinition\Data\QuantityValueRange)
&& $fkey === 'unit'
) {
try {
$this->db->executeQuery(
sprintf(
'ALTER TABLE `%s` ADD CONSTRAINT `%s` FOREIGN KEY (`%s`)
REFERENCES `quantityvalue_units` (`id`) ON DELETE SET NULL',
$table,
self::getForeignKeyName($table, $key . '__' . $fkey),
$key . '__' . $fkey
)
);
} catch (DriverException $e) {
// Ignore if the foreign key already exists
}
}
}
} else {
if ($value->getColumnType()) {
Expand Down
39 changes: 39 additions & 0 deletions models/DataObject/Objectbrick/Definition/Dao.php
View file
Open in desktop
Expand Up @@ -15,6 +15,7 @@

namespace Pimcore\Model\DataObject\Objectbrick\Definition;

use Doctrine\DBAL\Exception\DriverException;
use Pimcore\Db\Helper;
use Pimcore\Model;
use Pimcore\Model\DataObject;
Expand Down Expand Up @@ -104,6 +105,25 @@ public function createUpdateTable(DataObject\ClassDefinition $class): void
foreach ($value->getColumnType() as $fkey => $fvalue) {
$this->addModifyColumn($tableStore, $key . '__' . $fkey, $fvalue, '', 'NULL');
$protectedColumnsStore[] = $key . '__' . $fkey;

if (($value instanceof DataObject\ClassDefinition\Data\QuantityValue
|| $value instanceof DataObject\ClassDefinition\Data\QuantityValueRange)
&& $fkey === 'unit'
) {
try {
$this->db->executeQuery(
sprintf(
'ALTER TABLE `%s` ADD CONSTRAINT `%s` FOREIGN KEY (`%s`)
REFERENCES `quantityvalue_units` (`id`) ON DELETE SET NULL',
$tableStore,
self::getForeignKeyName($tableStore, $key . '__' . $fkey),
$key . '__' . $fkey
)
);
} catch (DriverException $e) {
// Ignore if the foreign key already exists
}
}
}
} elseif ($value->getColumnType()) {
$this->addModifyColumn($tableStore, $key, $value->getColumnType(), '', 'NULL');
Expand All @@ -121,6 +141,25 @@ public function createUpdateTable(DataObject\ClassDefinition $class): void
foreach ($value->getQueryColumnType() as $fkey => $fvalue) {
$this->addModifyColumn($tableQuery, $key . '__' . $fkey, $fvalue, '', 'NULL');
$protectedColumnsQuery[] = $key . '__' . $fkey;

if (($value instanceof DataObject\ClassDefinition\Data\QuantityValue
|| $value instanceof DataObject\ClassDefinition\Data\QuantityValueRange)
&& $fkey === 'unit'
) {
try {
$this->db->executeQuery(
sprintf(
'ALTER TABLE `%s` ADD CONSTRAINT `%s` FOREIGN KEY (`%s`)
REFERENCES `quantityvalue_units` (`id`) ON DELETE SET NULL',
$tableQuery,
self::getForeignKeyName($tableQuery, $key . '__' . $fkey),
$key . '__' . $fkey
)
);
} catch (DriverException $e) {
// Ignore if the foreign key already exists
}
}
}
} elseif ($value->getQueryColumnType()) {
$this->addModifyColumn($tableQuery, $key, $value->getQueryColumnType(), '', 'NULL');
Expand Down