Skip to content
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
* Copyright 2015 Adobe
* All Rights Reserved.
*/
namespace Magento\Framework\Setup\Declaration\Schema\Dto;

Expand All @@ -22,6 +22,8 @@ class ElementFactory
*
* Where @key - is xsi:type of the object
* Where @value - is instance class name
*
* @var array
*/
private $typeFactories = [];

Expand Down Expand Up @@ -70,6 +72,25 @@ private function castGenericAttributes(array $elementStructuralData)
return $elementStructuralData;
}

/**
* Remove empty comments from the schema declaration
*
* Empty comments are never persisted in the database, they always end up being read back as null
*
* @see \Magento\Framework\Setup\Declaration\Schema\Db\MySQL\DbSchemaReader::readColumns
*
* @param array $elementStructuralData
* @return array
*/
private function removeEmptyComments(array $elementStructuralData)
{
if (isset($elementStructuralData['comment']) && $elementStructuralData['comment'] === "") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider adding the edge cases like if the whitespace-only strings (" ") or "0" (which is technically a valid comment but falsy)

Copy link
Contributor Author

@convenient convenient Nov 19, 2025

Choose a reason for hiding this comment

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

Hello @engcom-Hotel

I'm not sure what you're seeing in the testing here, if someone wants to create a table with a comment 0 that's their prerogative. It adds cleanly to the database and has no issues in the comparison so not sure what you're seeing here.

It's not a common use case in the industry for people to define a comment with multiple space string chars, that's not a case I've seen in any extensions in the wild but i'll add trim to cater for that scenario

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the clarification! You're absolutely right about "0". A comment with the value "0" is perfectly valid and should be handled properly. My concern wasn't about "0" being falsy, but rather about ensuring the code explicitly handles this case as intended.

Regarding whitespace-only strings: I appreciate you adding trim() to handle that scenario. While it may not be common in the wild, defensive coding against such edge cases can prevent unexpected behavior in schema comparisons and improve code robustness.

Thanks for addressing the feedback!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @engcom-Hotel

The code seems to handle the case of 0 correctly, as it gets added to the database and also read back. Is there anything else you need at this stage to get this untagged from needs update? I think i have answered all queries?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes @convenient, thank you!

I am approving this PR for further processing. 👍

unset($elementStructuralData['comment']);
}

return $elementStructuralData;
}

/**
* Instantiate different types of elements, depends on their xsi:type.
*
Expand All @@ -84,6 +105,7 @@ public function create($type, array $elementStructuralData)
}

$elementStructuralData = $this->castGenericAttributes($elementStructuralData);
$elementStructuralData = $this->removeEmptyComments($elementStructuralData);
$elementStructuralData['type'] = $type;
return $this->typeFactories[$type]->create($elementStructuralData);
}
Expand Down